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 >
