RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Ivan Gerasimov
ivan.gerasimov at oracle.com
Tue Jul 23 08:06:12 PDT 2013
On 23.07.2013 18:50, Daniel D. Daugherty wrote:
> Yes. Do you have a pointer to the committed patch file?
> If so, I'll take care of getting the fix into JDK8-T&L.
Thanks a lot! I really appreciate it.
Here's the link:
http://cr.openjdk.java.net/~igerasim/2commit/8016838-jdk8-ReBigClass-improved.patch
I've just updated the file to include the latest changes.
Sincerely yours,
Ivan
> Dan
>
>
>> I've just checked how the tests run against the latest jdk build
>> (which is 99), and the leak is still there.
>> Thus the proposed change (including ProblemList modifications) is
>> still actual.
>>
>> Here's a link to the latest webrev:
>> http://cr.openjdk.java.net/~igerasim/8016838/3/webrev/
>>
>> I've also run jprt job to check how it works on all other platforms:
>> http://prt-web.us.oracle.com//archive/2013/07/2013-07-22-143846.igerasim.jdk/logs/
>>
>>
>> On all the platforms (including 32-bit Linux) the tests passed as
>> expected, on the Linux-x64 both tests were skipped.
>>
>> Virtual memory growth across {fastdebug,product}-{c1,c2} targets was
>> in range from 1188K to 2584K which is less than the chosen threshold
>> of 32M.
>>
>> Sincerely yours,
>> Ivan Gerasimov
>>
>>
>> On 10.07.2013 14:38, Ivan Gerasimov wrote:
>>> 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