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
