Would not ConcurrentHashMap.putIfAbsent() help with some of the duplicate
checking?
Gregg Wonderly
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.
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
