RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Seán Coffey
sean.coffey at oracle.com
Fri Jul 5 11:45:49 UTC 2013
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