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.back...@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 >>
