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
