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

[Ivan Gerasimov]

On 05.07.2013 8:35, Daniel D. Daugherty wrote:
> 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)?
>
I've only run test from java/lang/instrument when checked the change 
with JDK6u60 (all passed) and with JDK6u51 (the test failed as expected, 
since the leak had still been there.)

> 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.
Right. The test shown a memleak with the the latest jdk8.
I filed a bug 8019845 about this leak.
Stefan Karlsson guessed that this may be related to 8003419 (NPG: Clean 
up metadata created during class loading if failure)
Then I've checked the builds b57 (test failed) and b58 (test passed), so 
I confirmed that it may be the reason of the leak being observed now.
But now I think that the reason may be different.
It just turns out that the test shows failures for (at least) three 
different leaks - the one you, Daniel, solved (7121600), the one Stefan 
wrote about (8003419) and the one currently observed (8019845).

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

Well, the leak is there, and why not have a failing test as a reminder 
to have it fixed?

Sincerely yours,
Ivan Gerasimov

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