RFR [8016838] java/lang/instrument/RedefineBigClass.sh needs modification

Seán Coffey sean.coffey at oracle.com
Wed Jul 10 09:15:36 UTC 2013


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