RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Ivan Gerasimov
ivan.gerasimov at oracle.com
Wed Jul 10 03:38:22 PDT 2013
Yes, I forgot to include the most important thing - a link to webrev!
Your link is correct.
http://cr.openjdk.java.net/~igerasim/8016838/3/webrev/
The tests fail on linux-x64 only, linux-i586 is fine.
Here's the link to the logs of the tests run by Daniel Daugherty:
http://prt-web.us.oracle.com//archive/2013/07/2013-07-05-045326.ddaugher.8016838_exp/logs/
Thus the memory leak seems to be specific to 64-bit Linux.
Sincerely yours,
Ivan
On 10.07.2013 13:15, Seán Coffey wrote:
> Ivan,
>
> I'll assume this is the latest webrev :
> http://cr.openjdk.java.net/~igerasim/8016838/3/webrev/
> <http://cr.openjdk.java.net/%7Eigerasim/8016838/3/webrev/>
>
> One comment - should the test be excluded for all linux variants (i.e.
> linux-all) ?
>
> regards,
> Sean.
>
> On 09/07/2013 14:09, Ivan Gerasimov wrote:
>> Please have a chance to review an updated webrev.
>> It now includes a change to ProblemList.txt, so both modified tests
>> are ignored for linux-x64.
>>
>> Sincerely yours,
>> Ivan Gersimov
>>
>> On 08.07.2013 21:27, Seán Coffey wrote:
>>>
>>> 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 serviceability-dev
mailing list