RFR S: 7127792 Add the ability to change an existing PeriodicTask's execution interval
Rickard Bäckman
rickard.backman at oracle.com
Thu Oct 11 23:18:03 PDT 2012
Thank you Markus.
/R
On Oct 12, 2012, at 7:29 AM, Markus Grönlund wrote:
> I am ok with this change Rickard.
>
> Thanks also to David Holmes for his great feedback and help on this one.
>
> /Markus
>
> Sent from my iPhone
>
> On 12 okt 2012, at 07:06, Rickard Bäckman <rickard.backman at oracle.com> 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
>>
More information about the serviceability-dev
mailing list