Pls (re)review 6173675/7003271: per-thread memory allocation measurement

[Paul Hohensee]

I'll file a CR.

Thanks for your review!

Pul

On 1/10/11 2:01 AM, David Holmes wrote:
> Paul Hohensee said the following on 01/10/11 16:42:
>> My bad on the 7003271 webrev.  It's already pushed.  The only one to
>> review is 6173675.
>>
>> I replaced initialLongArray with java.util.Arrays.fill.
>>
>> I took the code for ThreadCpuTimeArray.java from
> test/java/lang/management/
>> ThreadCpuTime.java. The check for DELTA was in that code, so I kept 
>> it. Same with the argument of 200 to goSleep(). I'm assuming that the
>> same logic works, and it appears to do so. I can, though, change it
>> to be like the code in ThreadAllocatedMemoryArray.java.
>
> I'd say both tests using the DELTA are equally dubious, but I'd need 
> to know what the intent of that check was. If it was to see if the cpu 
> time advanced by an expected amount then a better test would be for 
> each thread to measure elapsed time around doit() and then have the 
> main thread check that the elapsed CPU time is <= the measured elapsed 
> time. (Some of the JVMTI test do similar comparisons).
>
> That said, given you are emulating existing tests that appear to work 
> I won't push for you to re-write them :) But we should have a CR to 
> either change them or at least make the DELTA a configurable value. I 
> can easily imagine that these tests will fail on some of our internal 
> platforms.
>
> Cheers,
> David
>
>>
>> Paul
>>
>> On 1/9/11 9:17 PM, David Holmes wrote:
>>> Hi Paul,
>>>
>>> Partial re-review :)
>>>
>>> Paul Hohensee said the following on 01/10/11 11:10:
>>>> Thanks to all who reviewed the previous webrev.  Revised versions 
>>>> are here
>>>>
>>>> http://cr.openjdk.java.net/~phh/6173675/webrev.02/
>>>
>>> In ThreadImpl.java you could replace:
>>>
>>>     long[] times = initialLongArray(length, -1);
>>>
>>> with
>>>
>>>     long[] times = java.util.Arrays.fill(new long[length], -1);
>>>
>>> and delete initialLongArray.
>>>
>>>
>>> In test/com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java
>>>
>>> I don't understand the test logic. Your tests threads all do:
>>>
>>> doit();
>>> while (!done) wait
>>> doit();
>>>
>>> but the main thread doesn't check any times etc after setting 
>>> done=true, so why have the test threads do the second call to doit? 
>>> For that matter why even bother with doit()? It only delays the 
>>> threads in getting to the wait(), which is what the main thread 
>>> itself waits for.
>>>
>>>  111         goSleep(200);
>>>
>>> What does 200 represent? What is it that you are actually waiting 
>>> for? Is 200 long enough on a slow uniprocessor?
>>>
>>>  141             if ((times[i] + DELTA) < newTime) {
>>>  142                 throw new RuntimeException("TEST FAILED: " +
>>>
>>> Again what does DELTA represent, how can you be sure the times for 
>>> all threads will have advanced this much?
>>>
>>>
>>>> http://cr.openjdk.java.net/~phh/7003271/webrev.02/
>>>
>>> All files listed have zero changes ???
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> The interesting changes are in the library side for 6173675.  In 
>>>> particular,
>>>> I've added 3 unit tests.
>>>>
>>>> Thanks for any review,
>>>>
>>>> Paul
>>>>