RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Ivan Gerasimov
ivan.gerasimov at oracle.com
Mon Jul 8 16:55:47 UTC 2013
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.
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