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 requirements
Date Sun, 18 Jul 2010 17:47:29 GMT
Good point. I have a bit of a blind spot in this area because my current 
strategy is to first get TaskManager scaling under control, and thread 
management correct, with a KISS approach to concurrency correctness. 
After that is done, I'm planning some more measurement that may lead to 
either more aggressive optimization of specific methods or to dividing 
the synchronization up to allow portions of a TaskManger to run in 
parallel. This one is simple and clean enough that I could do it now 
with no real cost.

However, I've been thinking about race conditions in connection with 
addIfNew, and now have serious doubts about whether it should exist at 
all. It has only one use, so the design question is whether the 
duplicate task testing should be done in TaskManager or in the single 
caller, RegisterImpl.

The Task run method is called, of course, without TaskManager 
synchronization. When a task finishes there is a time window between 
completion of the Task run method and the TaskManager cleanup, including 
removal from whatever structure controls addIfNew. Even if it were 
synchronized on the TaskManger, there is nothing preventing an addIfNew 
call for an equal Task in that window.

It may all be correct, for example if RegisterImpl permanently remembers 
the results of an AddressTask run, but the code I would need to review 
to check that is in RegisterImpl, not TaskManager. As is implied by your 
suggestion, the whole issue is independent from the rest of TaskManager. 
Moreover, I'm not sure I can clearly describe the requirements for using 
addIfNew correctly. That is important because I want to make the API 
documentation clearer and more complete on concurrency issues.

To me, this all seems as though the if-new test is more closely coupled 
to RegisterImpl than to TaskManager.

Patricia


On 7/18/2010 8:01 AM, Gregg Wonderly wrote:
> HashSet is not concurrent.  Would it be better to just use ConcurrentHashMap to eliminate
the use of synchronize?
>
> Gregg Wonderly
>
> Sent from my iPhone
>
> On Jul 17, 2010, at 9:49 PM, Peter Firmstone<jini@zeus.net.au>  wrote:
>
>> Sounds like a good optimisation, I'm for it.  I agree, don't but all the tasks in
the HashSet, just those you don't want duplicated.
>>
>> Patricia Shanahan wrote:
>>> TaskManager has a method addIfNew(Task t) that adds a Task if, and only if, there
is no existing task that it equals, according to .equals comparison.
>>>
>>> It has one existing use, adding an AddressTask in com.sun.jini.reggie.RegistrarImpl.
All instances of AddressTask are added using addIfNew. AddressTask is final, and an AddressTask
considers itself equal only to other instances of AddressTask.
>>>
>>> I would like to modify TaskManager so that addIfNew adds the new Task unless
there is an equal Task that was also added using addIfNew. The new javadoc comment would read:
>>>
>>>     /**
>>>      * Add a new task if it is not equal (using the equals method)
>>>      * to any existing active or pending task that was also added
>>>      * by this method.
>>>      */
>>>
>>> This permits a very simple implementation that does not depend on an O(n) scan
of all existing tasks. addIfNew would add the Task to a HashSet<Task>, and do the rest
of the processing if, and only if, the HashSet add method returns true. I would remember if
a particular Task came through addIfNew, and if it did it will be removed from the HashSet
on completion or removal.
>>>
>>> This implementation would fully meet the needs of the existing use, at least
once I improve the hashCode method in AddressTask.
>>>
>>> The problem with putting all added Task instances in the HashSet is that the
add method permits addition of multiple tasks that equal each other. I could work around that
problem, but it would make the code significantly more complicated, and add work to the normal
case of unconditionally added tasks.
>>>
>>> Patricia
>>>
>>
>


Mime
View raw message