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

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Jul 23 14:50:16 UTC 2013


Ivan,

Sorry for forgetting about this issue...


On 7/23/13 8:14 AM, Ivan Gerasimov wrote:
> Hello Daniel!
>
> Can we please move forward with this issue?

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.

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 core-libs-dev mailing list