river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patricia Shanahan <p...@acm.org>
Subject Re: TaskManager progress
Date Wed, 21 Jul 2010 22:02:48 GMT
On 7/21/2010 12:58 PM, Gregg Wonderly wrote:
> Here are some thoughts/questions as I look at the code.  Thanks for
> taking this task on!

Thanks for your comments. After several years of working on individual 
projects with no code review, it is refreshing to be back in an 
environment in which other programmers will look at my code and comment 
on it.

One general conclusion from your comments is that I had design rules in 
my mind that I forgot to write down. That may be a bad habit I got into 
while I was the only reader of my code. It is not reasonable to expect 
you to read my mind, rather than the code and its comments, to find out 
how it is intended to work.

> in add(Task)
> is the order of taskToWrapper.put() vs addTasks.add() important
> compared to the use of the contents of those? Is this an atomic
> relationship that needs to involve some logic to make the 'view'
> atomic in nature, such as reordering these two statements?
> In TaskWrapper.endTask() the order is reversed and this implies
> that the order of these statements is important to your design,
> so I just want to make sure I understand their relationship.

Both structures are supposed to be kept consistent, and only accessed or 
modified in code that is synchronized on the TaskManager. I feel it is a 
bit tidier, even in single thread code, to unwind things backwards from 
setting them up, but I don't feel strongly about that and will change 
the order in one place or the other if you feel it would enhance 

> in remove(TaskWrapper,boolean)
> Can there be a transition of states between the time the switch
> statement is started and the time the specific case executes?
> Do we need a lock here to hold the thread in its current state
> so that WAITING and READY threads are correctly "stopped"?

A TaskWrapper's state should only be accessed or changed in code that is 
synchronized on the TaskManager.

> --runnable and ++runnable
> This is not concurrency safe. I'd suggest an AtomicInteger instead,
> especially if there is no other reason to use "synchronized" where
> this is done. Visibility needs to be guaranteed using some happens
> before as well.

runnableTasks should only be accessed or changed in code that is 
synchronized on the TaskManager.

> When I write code of this nature, attempting to remove all contention, I
> try
> to list every "step" that changes the "view" of the world, and think about
> how that "view" can be made atomic by using explicit ordering of statements
> rather than synchronized{} blocks. Visibility still has to work, so one
> also
> needs to worry about "happens before" as well. This looks like a really
> good
> start on the algorithmic steps. Hopefully, some others can look things
> over and contribute any other issues or improvements they have.

I feel that avoiding "synchronized" often makes reasoning about 
concurrency and visibility more complicated than it needs to be. I first 
want to make TaskManager operations reasonably efficient. Only after 
that, I'll consider making them more parallel, if benchmarking indicates 
it is useful to do so.

For a TaskManager holding n tasks, there are only two O(n) operations 
left. This should reduce the need for parallelism, compared to the 
ArrayList based design. One of them, construction of a list of pending 
tasks, is inherently O(n) and probably infrequent. The other is the 
runAfter scan. Everything else is no worse than O(log n). The area where 
parallelism is most likely to be useful is in the runAfter scans.

My intent is to make this a very simple case for concurrency and 
visibility issues. With one exception, all changes to the shared data 
structures should be in blocks that are synchronized on the TaskManager, 
with all steps needed for a logical change done without releasing 
synchronization. Anything that is done in a block that is synchronized 
on a given TaskManager happens-before any subsequent action that is 
synchronized on the same TaskManager instance.

The exception is the physical removal of a TaskWrapper from the 
readyTasks queue. The poll call has to be outside any block that is 
TaskManager synchronized, so the TaskRunner has to recheck terminated 
and the status of the TaskWrapper after obtaining TaskManager 
synchronization but before changing the status of the TaskWrapper to 
RUNNING. The PriorityBlockingQueue looks after its own concurrency and 
visibility issues.

In considering your comments I found a bug in this area. If the status 
of a TaskWrapper is changed to REMOVED in the timing window between 
getting it from the queue and getting synchronization, it has already 
been wrapped up by the remove code. Logically, the task is out of the 
TaskManager. The TaskWrapper is just hanging on by the reference in the 

Which would be better, slapping "synchronized" on a private method that 
I only intend to call from synchronized code, or adding a comment 
documenting the intent?

Thanks again for your comments.


> Gregg Wonderly
> Patricia Shanahan wrote:
>> I've uploaded a new version with tabs for indentation.
>> Patricia
>> On 7/21/2010 9:33 AM, Patricia Shanahan wrote:
>>> No problem. That sort of thing I'll handle later by reformatting after
>>> setting up the project settings in Eclipse.
>>> Patricia
>>> On 7/21/2010 8:55 AM, Gregg Wonderly wrote:
>>>> One of my most desired attributes of code is that tabs be used in place
>>>> of spaces. The reason for this, is so that I can change tab expansion,
>>>> on the fly, to narrow or widen the view of nested blocks to help me
>>>> better see what is there.
>>>> This is a religious kind of issue, and I know there are countless
>>>> people
>>>> who think otherwise. As a VI user, I, countless times, have typed ':set
>>>> ts=4 sw=4'
>>>> and ':set ts=8 sw=8' in code to change my viewpoint.
>>>> I know that others have reasons why they prefer spaces. I've just never
>>>> been able to find any override factors that make spaces a good choice,
>>>> especially when you are in an editor without a mouse.
>>>> Gregg Wonderly
>>>> Peter Firmstone wrote:
>>>>> Thanks Patricia, looking good, will take some time to digest it
>>>>> further.
>>>>> We don't have a set of coding conventions, unless someone wants to
>>>>> write a tool, there used to be one in com.sun.jini.tool, as evidenced
>>>>> by one of the jtreg tests
>>>>> trunk/qa/jtreg/com/sun/jini/tool/CheckCodeStyle
>>>>> Perhaps there are some widely available tool that we could settle on?
>>>>> I like to follow Kent Beck's style in his book Implementation Patterns
>>>>> ISBN-10 0-321-41309-1, it's quite a small book and makes easy reading,
>>>>> but that's just my personal preference.
>>>>> Cheers,
>>>>> Peter.
>>>>> Patricia Shanahan wrote:
>>>>>> I've uploaded my current work-in-progress code as
>>>>>> http://www.patriciashanahan.com/apache/NewTaskManager.java
>>>>>> Please send me any comments, questions, or suggestions for
>>>>>> improvement.
>>>>>> The change of name is temporary, to allow a smoother transition.
>>>>>> plan to work through the callers, changing them one at a time to
>>>>>> the new Task interface. When they have all been changed, and there
>>>>>> are no more TaskManager references, the name can be changed to
>>>>>> TaskManager.
>>>>>> I'll need to set up the correct formatting in Eclipse, but once I
>>>>>> find the rules that won't take long. Any other coding conventions
>>>>>> need to watch out for?
>>>>>> Meanwhile, I'm working on more testing and benchmarking. It
>>>>>> definitely improves performance when there are a lot of tasks or
>>>>>> runAfter dependencies, but I need to do more testing for short tasks
>>>>>> in simple cases, the case in which it is most likely to be worse
>>>>>> the current code.
>>>>>> Patricia
>>>>>> On 7/20/2010 2:48 PM, Peter Firmstone wrote:
>>>>>>> Looking forward to seeing some code. SVN builds clean again.
>>>>>>> Patricia Shanahan wrote:
>>>>>>>> I did the first tests of my new TaskManager today. I can't
>>>>>>>> benchmark
>>>>>>>> very accurately because of a QA test running on the same
>>>>>>>> but
>>>>>>>> it seems to be about the same without dependencies, and
>>>>>>>> significantly
>>>>>>>> faster with dependencies. Specifically, it removes the single
>>>>>>>> bottleneck.
>>>>>>>> I'll next do more testing, benchmarking, and tuning in my
>>>>>>>> environment.
>>>>>>>> Patricia

View raw message