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

Seán Coffey sean.coffey at oracle.com
Mon Jul 8 17:27:56 UTC 2013


On 08/07/13 17:55, Ivan Gerasimov wrote:
> Thanks, Seán!
>
> I located the build in which the memleak was first introduced -- it is 
> jdk8-b69 (hs25-b13).
> I've updated the bug http://bugs.sun.com/view_bug.do?bug_id=8019845 
> with this.
>
> So what is the correct procedure to go forward now?
> Should I update the webrev to include changes to the problem list?
> I believe I shouldn't -- this list seems to be a sensitive stuff.
I'd suggest updating the webrev with the ProblemList 
modification/addition. It's best not to add a test to testruns if it's 
knowingly failing. The test can be removed from ProblemList when the 
jdk8 regression is fixed.

regards,
Sean.

>
> Sincerely yours,
> Ivan
>
>
> On 05.07.2013 15:45, Seán Coffey wrote:
>> Nice work indeed Ivan. Good to have a reliable testcase to catch 
>> leaks in this area.
>>
>> I'd also suggest that this test goes on the ProblemList until the new 
>> leak is sorted out for jdk8. The goal of JPRT runs is to have clean 
>> runs.  If it's on the problemList, then it's a known issue and is 
>> normally tagged with the relevant bug ID so that the responsible 
>> engineer knows the current state.
>>
>> regards,
>> Sean.
>>
>> On 05/07/2013 11:53, Ivan Gerasimov wrote:
>>>
>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>




More information about the core-libs-dev mailing list