Daniel, thank you for review!
Here's the updated with all all your suggestions adopted.
http://cr.openjdk.java.net/~igerasim/8016838/1/webrev/
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?
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
