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

[Paul Hohensee]

I filed 7011788 for this.

Paul

On 1/10/11 2:29 AM, Paul Hohensee wrote:
> 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
>>>>>