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

[Daniel D. Daugherty]

Ivan,

The changes look fine, I can sponsor your commit, looks like your
OpenJDK user name is 'igerasim', but I need to know a little bit
more about your testing of these fixes. Did you do a test JPRT
job to run the JLI tests (or just the two tests themselves)?

Based on e-mail about this bug fix, I believe you've found a new
leak in JDK8/HSX-25 with test java/lang/instrument/RedefineBigClass.sh.
That's a good thing, but I think Alan and company would be a bit
grumpy with me if I pushed a test that failed as soon as someone
ran it. :-) Do you know if the revised RetransformBigClass.sh also
finds a new memory leak in JDK8/HSX-25?

The way to deal with that is to put the test on the Problem.list
as part of the same changeset. However, the T&L folks might not like
that either...

Dan


On 7/4/13 6:59 PM, Ivan Gerasimov wrote:
> Thank you, Daniel!
>
> Please find an updated webrev at 
> http://cr.openjdk.java.net/~igerasim/8016838/2/webrev/.
> It now includes the RetransformBigClass test modified in the same way 
> as RedefineBigClass
> If the changes look fine, may I ask you to sponsor the commit, as I'm 
> not a committer?
> Here's a link to hg export:
> http://cr.openjdk.java.net/~igerasim/2commit/8016838-jdk8-ReBigClass-improved.patch 
>
>
> Thanks in advance,
> Ivan
>
> On 04.07.2013 21:45, Daniel D. Daugherty wrote:
>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>