river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Firmstone <j...@zeus.net.au>
Subject Re: TaskManager progress
Date Thu, 22 Jul 2010 00:25:55 GMT
Hi Patricia,

Instead of Task passing an Iterable<Task>, it could pass an 
Enumerator<Task>, which is inherently immutable.  One more comment inline.

Patricia Shanahan wrote:
> 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 readability.
>
>> 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 TaskRunner.
>
> 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?

Add a comment documenting the intent ;)

>
> Thanks again for your comments.
>
> Patricia
>
>
>
>>
>> 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.
I
>>>>>>> plan to work through the callers, changing them one at a time
to 
>>>>>>> use
>>>>>>> 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
I
>>>>>>> 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

>>>>>>> than
>>>>>>> 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

>>>>>>>>> computer,
>>>>>>>>> but
>>>>>>>>> it seems to be about the same without dependencies, and
>>>>>>>>> significantly
>>>>>>>>> faster with dependencies. Specifically, it removes the
single 
>>>>>>>>> task
>>>>>>>>> bottleneck.
>>>>>>>>>
>>>>>>>>> I'll next do more testing, benchmarking, and tuning in
my own
>>>>>>>>> environment.
>>>>>>>>>
>>>>>>>>> Patricia
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>


Mime
View raw message