RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Seán Coffey
sean.coffey at oracle.com
Wed Jul 10 11:39:57 UTC 2013
On 10/07/13 11: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.
Ok - didn't know that. Thanks. looks fine to me then, but use Dan or
someone else as reviewer since I'm not one.
regards,
Sean.
>
> 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 core-libs-dev
mailing list