Rickard,
Are David and Marcus sufficient to push this. I briefly looked at it a
few times and have to admit not knowing anything about this code without
further study. Dan and Karen probably know more. Keith might too,
so I've cc'd him. Sorry I can't be more helpful but if you need a
reviewer before the end of your day and nobody else responds, you can
use my name. I can be the default reviewer in this case.
Coleen
On 10/12/2012 1:06 AM, Rickard Bäckman wrote:
People,
I need at least one more reviewer, thanks!
/R
On Oct 9, 2012, at 3:00 PM, Rickard Bäckman wrote:
David,
thanks for your review!
/R
On Oct 9, 2012, at 2:01 PM, David Holmes wrote:
On 9/10/2012 9:42 PM, Rickard Bäckman wrote:
David,
see inline.
On Oct 9, 2012, at 1:18 PM, David Holmes wrote:
On 9/10/2012 7:36 PM, Rickard Bäckman wrote:
David,
thanks for your reply!
I've changed the code according to the suggestions, I've also changed the types
in PeriodicTask from being a size_t to being a
jint (see updated webrev for details).
But now the type changes have been pushed out to the task creators. Most task
creations pass an int already but BiasedLocking uses a size_t. Just shows how
messed up the typing was to begin with.
Agreed. Two ways of solving it, 1) change the callers to use an int. 2) Do the
cast in the constructor (should be safe since we check the possible interval).
int -> size_t shouldn't cause a warning so callers currently passing int are
ok. So keeping it as size_t in constructor arg and casting to int before storing
internally seems okay.
Minor nit: should be an int rather than jint as these are not Java types.
Will fix.
To prevent the waiting for very long time (which could overflow, etc) when we
don't have any active task, I added an extra if
so that if we are waiting while no tasks are available, we reset the
time_before_wait and consider time_slept to be zero after sleeping.
That means the first task added will always sleep for the period requested.
Those semantics seem reasonable.
The only thing that concerns me here is the affect of calling
real_time_tick(0). I can't quite tell what the profiling code does.
I could avoid calling that if time_slept = 0
Ok. You could skip real_time_tick altogether on zero.
I'm generally okay with the approach being taken here, but the changes are
disruptive enough that I can't see for sure that existing tasks will be
unaffected. I guess time will tell. And let's see what others might spot.
Thanks,
David (signing off for the night)
Thanks
/R
David
-----
Updated webrev:
http://cr.openjdk.java.net/~rbackman/7127792.1/
Thanks
/R
On Oct 9, 2012, at 9:46 AM, David Holmes wrote:
Sorry Rickard, missed the original RFR :)
So to be clear here the synopsis concerns changing the period, while the
mechanism implemented is for a more general dynamic disenroll / enroll. So
changing a period is effected by removing a task and then adding it with the
new period.
And for anyone not reading the fine-print when you dynamically enroll a task
its first firing is somewhat arbitrary - somewhere between the time of
enrollment and that time plus its period.
src/share/vm/runtime/thread.hpp
Can you add a comment:
static void stop();
+ // Only allow start once the VM is sufficiently initialized
+ // Otherwise the first task to enroll will trigger the start
+ static void make_startable();
---
src/share/vm/runtime/thread.cpp
We have a bit of type mixing here:
- size_t time_to_wait
- jlong time_slept
- int remaining = time_to_wait
I don't understand why Task is using size_t for time intervals. If you make "remaining" a
size_t then it will cause issues when you pass it to wait. But I would expect the initialization of
"remaining" to cause an unsigned-to-signed conversion warning, so perhaps an explicit
cast to silence that.
1203 bool status = PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag,
remaining);
1204 if (status || _should_terminate) {
1205 break;
1206 }
Can you rename status to timedout to make the logic more obvious. Also note that you will
potentially return with time_slept still at zero, even though you may have slept for an
arbitrary amount of time. That seems wrong as zero will then be passed to
"tick".
1208 // spurious wakeup of some kind
This comment is no longer accurate as you may have been woken up due to a
change in the task list, I suggest:
// Change to task list or spurious wakeup of some kind
1213 remaining = PeriodicTask::time_to_wait();
1214 if (remaining == 0) {
1215 continue;
1216 }
Can you insert a comment before continue:
// Last task was just disenrolled so loop around and wait until
// another task gets enrolled
continue;
1218 remaining -= time_slept;
Again type mixing: subtracting a long from an int. Again a potential warning to
get rid of.
Also in a long running VM perhaps there have been no periodic tasks for many
days and then one turns up. The subtraction could wrap and cause remaining to
remain positive. I know you've now documented the uncertainty in the first
fire time but this seems somewhat too random. Let's see what others think. :)
1329 PeriodicTask_lock->notify_all();
There is only one thread that waits so notify() will suffice.
Thanks,
David
On 9/10/2012 4:34 PM, Rickard Bäckman wrote:
Trying again,
can I have a couple of reviews, please?
/R
On Oct 4, 2012, at 3:01 PM, Rickard Bäckman wrote:
Hi all,
can I please have a couple of reviews on the following change:
http://cr.openjdk.java.net/~rbackman/7127792/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127792
In short the purpose is to enable tasks to change the interval they are
executed in. We
would also like to be able to add (and remove) tasks after the WatcherThread
has started.
Thanks
/R
