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

[Daniel D. Daugherty]

On 7/4/13 11:19 AM, Ivan Gerasimov wrote:
> Daniel, thank you for review!
>
> Here's the updated with all all your suggestions adopted.
> http://cr.openjdk.java.net/~igerasim/8016838/1/webrev/

Looks good.


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

I would include it in the same changeset.

Dan


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