RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification
Ivan Gerasimov
ivan.gerasimov at oracle.com
Tue Jul 9 06:09:32 PDT 2013
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