RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java didn't check Thread.sleep

[Jaroslav Bachorik]

On 23.10.2013 02:40, David Holmes wrote:
> On 22/10/2013 9:03 PM, Jaroslav Bachorik wrote:
>> On 22.10.2013 09:58, David Holmes wrote:
>>> On 21/10/2013 9:55 PM, Jaroslav Bachorik wrote:
>>>> Please, review this small patch for a test failing due to the updated
>>>> implementation in JDK6.
>>>>
>>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-6309226
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.00/
>>>>
>>>> The test fails due to the change in mustang where
>>>> ThreadMXBean.getThreadInfo().getWaitedTime() and
>>>> ThreadMXBean.getThreadInfo().getWaitedCount() include Thread.sleep()
>>>> too. Unfortunately, Thread.sleep() is used throughout the test for
>>>> synchronization purposes and this breaks the test.
>>>>
>>>> In the patch I propose to replace Thread.sleep() with busy wait and
>>>> hinting the scheduler by Thread.yield(). While not very elegant it
>>>> successfully works around inclusion of unknown number of
>>>> Thread.sleep()s
>>>> (they are called in loop).
>>>
>>> Not elegant and not completely reliable either. Probably adequate on a
>>> multi-core system but single-core and with some schedulers it could just
>>> be a busy spin.
>>
>> :/ Ok, so I need to account for the Thread.sleep() calls made outside of
>> the test code but still reported by the ThreadMXBean. Not that elegant,
>> too, but at least should be reliable.
>>
>> http://cr.openjdk.java.net/~jbachorik/6309226/webrev.01
>
> Sorry, I can't follow the logic of that test enough to determine whether
> this compensating logic is correct.

It simply adds the number of times and the time spent in sleeping during 
calls to goSleep() from the BlockedThread (the one that actually counts).

It seems to be correct - otherwise the test would fail because the 
numbers wouldn't match.


>
> Whether this is more reliable depends on whether the 5% tolerance in
> timeRangeCheck is enough to account for all the potential inaccuracies
> in the time measurements. On a lightly loaded system it may be, but
> otherwise ... a context switch after determining the base time and doing
> the sleep could add an arbitrary load and cpu-dependent delay. It might
> be less reliable than the yield approach :(

I wonder how would "yield" in busy wait behave on a single core 
architecture. I need the second thread to progress while busy-waiting ...

>
> I can't help wonder whether there is a more explicit synchronization
> mechanism that will avoid the need for goSleep? But that becomes a much
> bigger task to deal with.

Yes. The only task of this fix is to enable the test to be run even 
after Thread.sleep() started to be included in the waited time (sometime 
in JDK6 timeframe). I suppose the test was successfully used before the 
change and if there are any problems with timing additional issues will 
be filed and the test will be redesigned.

For now I would like to keep the change simple and really focus on 
making the test executable on JDK7/8.

>
> I will leave this for the serviceability team to determine the best
> course of action.

Thanks for valuable comments, anyway.

-JB-

>
> Thanks,
> David
>
>> -JB-
>>
>>>
>>> David
>>>
>>>> Thanks,
>>>>
>>>> -JB-
>>