Patricia Shanahan wrote:
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.
It might be better to change RegisterImpl to check if it has been run
already with a volatile boolean, and if so return immediately.
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<[email protected]> 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
