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

[David Holmes]

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
>>>