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
