RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification

[Ivan Gerasimov]

Daniel, thank you for review!

Here's the updated with all all your suggestions adopted.
http://cr.openjdk.java.net/~igerasim/8016838/1/webrev/

I haven't yet considered applying the approach to RetransformBigClass.
Do you want me to include this into this same change set or should I 
make it separately?

Sincerely yours,
Ivan


On 04.07.2013 19:34, Daniel D. Daugherty wrote:
> On 7/3/13 11:12 AM, Ivan Gerasimov wrote:
>> Hello everybody!
>>
>> We have a request to improve jtreg test.
>> The test had been written to verify fix for memory leak during class 
>> redefinition.
>> The problem is that it always is reported as PASSED even in the 
>> presence of the leak.
>>
>> The proposed change is platform specific.
>> It allows memory leak detection only on Linux.
>> This is because the memory leak was in native code, so there's no 
>> easy way to detect it from within Java code.
>>
>> Here's the webrev for JDK8 repository:
>> http://cr.openjdk.java.net/~igerasim/8016838/0/webrev/
>
> Very nicely done! The logic in RedefineBigClass.sh only catches a
> leak big enough to cause an Exception to be thrown.
>
> When I originally wrote this test and its companion:
>
>     test/java/lang/instrument/RetransformBigClass*
>
> I could not come up with a platform independent way to put a small
> upper limit on memory growth. It never dawned on me that putting in
> something on one platform (Linux) was better than nothing.
>
> Are you planning to add this same logic to RetransformBigClass*?
>
>
>
> test/java/lang/instrument/RedefineBigClass.sh
>     No comments.
>
> test/java/lang/instrument/RedefineBigClassApp.java
>     line 48: final long MEM_LEAK_THRESHOLD = 32 * 1024; // 32Mb
>         Would be better at the top of RedefineBigClassApp rather than
>         "buried" down here.
>
>     line 51: Long.valueOf(vmMemDelta)
>         There are several places where a long is passed to 
> Long.valueOf().
>         Is there some reason you're not passing them directly to 
> println()?
>
>     line 54: } else {
>         The "else" is redundant due to the "System.exit(1)" call above 
> it.
>         You can drop the "else {" and "}" and shift that last 
> println() to
>         the left.
>
>     line 71: return Long.parseLong(line.split(" ")[22]) / 1024;
>         How about at least a comment referring to the Linux proc(5)
>         man page... and the fact that the 23rd field is:
>
>             vsize %lu   Virtual memory size in bytes.
>
> Again, very nicely done!
>
> Dan
>
>
>>
>> The surprising thing is that modified test detected the leak with the 
>> latest JDK8!
>> Latest 6 and 7 are fine, so it might be regression in JDK8.
>>
>> I've filled a bug http://bugs.sun.com/view_bug.do?bug_id=8019845 
>> "Memory leak during class redefenition" (not yet available.)
>>
>> Sincerely yours,
>> Ivan Gerasimov
>>
>>
>>
>
>
>