RFR: 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0 I've not figured a good way to write a regression test, but manually verified the leak and the fix by adding a microbenchmark that triggers the leak. When run with `-XX:NativeMemoryTracking=summary -XX:+UnlockDiagnosticVMOptions -XX:+PrintNMTStatistics` there will be a section of memory of unknown origin, e.g.: - Unknown (reserved=65536KB, committed=55104KB) (mmap: reserved=65536KB, committed=55104KB) With the patch, there is no memory unaccounted for. ------------- Commit messages: - Add code used to generate the X_LONG_NAME_BYTECODE - Add micros showcasing leak - Resolve memory leak in defineClass0 Changes: https://git.openjdk.java.net/jdk/pull/2407/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2407&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261154 Stats: 88 lines in 2 files changed: 84 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2407.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2407/head:pull/2407 PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 14:23:56 GMT, Claes Redestad <redestad@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920
Hi Cleas, looks good but why changing the the NULL comparisons? If you wanted to simplify you could completely omit the NULL checks since free(NULL) is a noop. And potentially merge the labels - where there are two - into one. Cheers, Thomas ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 15:48:16 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920
Hi Cleas,
looks good but why changing the the NULL comparisons?
If you wanted to simplify you could completely omit the NULL checks since free(NULL) is a noop. And potentially merge the labels - where there are two - into one.
Cheers, Thomas
I'm not sure every platform have always agreed free(NULL) is a noop. I suspect all the currently supported ones do, though? ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 15:53:24 GMT, Claes Redestad <redestad@openjdk.org> wrote:
I'm not sure every platform have always agreed free(NULL) is a noop. I suspect all the currently supported ones do, though?
Its standard behavior. Posix: https://pubs.opengroup.org/onlinepubs/009695399/functions/free.html "If ptr is a null pointer, no action shall occur. " Windows: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/free?view=m... " If memblock is NULL, the pointer is ignored and free immediately returns. " So it should be okay. ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 16:57:37 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
I'm not sure every platform have always agreed free(NULL) is a noop. I suspect all the currently supported ones do, though?
I'm not sure every platform have always agreed free(NULL) is a noop. I suspect all the currently supported ones do, though?
Its standard behavior.
Posix: https://pubs.opengroup.org/onlinepubs/009695399/functions/free.html "If ptr is a null pointer, no action shall occur. " Windows: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/free?view=m... " If memblock is NULL, the pointer is ignored and free immediately returns. "
So it should be okay.
Right. I dialed back the changes here since this is a bug fix that should be kept minimal. I have another change in the pipeline that will touch code here that can double down on cleaning up. ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 N = 200 Maximum resident set size (kbytes): 2124296 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920 N = 200 Maximum resident set size (kbytes): 921272
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Dial back cleanups and focus patch on the bug at hand ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2407/files - new: https://git.openjdk.java.net/jdk/pull/2407/files/972696ec..f25b5f98 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2407&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2407&range=00-01 Stats: 10 lines in 1 file changed: 0 ins; 5 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/2407.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2407/head:pull/2407 PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 16:14:57 GMT, Claes Redestad <redestad@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 N = 200 Maximum resident set size (kbytes): 2124296 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920 N = 200 Maximum resident set size (kbytes): 921272
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Dial back cleanups and focus patch on the bug at hand
LGTM. ------------- Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 16:14:57 GMT, Claes Redestad <redestad@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 N = 200 Maximum resident set size (kbytes): 2124296 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920 N = 200 Maximum resident set size (kbytes): 921272
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Dial back cleanups and focus patch on the bug at hand
Looks good! ------------- Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 17:07:01 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Dial back cleanups and focus patch on the bug at hand
Looks good!
@tstuefe, @ChrisHegarty, @mlchung - thanks for reviewing! ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 16:14:57 GMT, Claes Redestad <redestad@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 N = 200 Maximum resident set size (kbytes): 2124296 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920 N = 200 Maximum resident set size (kbytes): 921272
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Dial back cleanups and focus patch on the bug at hand
Looks good. Thanks for fixing this. ------------- Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2407
On Thu, 4 Feb 2021 14:23:56 GMT, Claes Redestad <redestad@openjdk.org> wrote:
This patch resolves a potential memory leak in Java_java_lang_ClassLoader_defineClass0
I've not figured a good way to write a regression test. A crude way to manually verify the leak and the fix is provided by the added microbenchmark that triggers the malloc in ClassLoader.c and the associated leak.
E.g., running this with `/usr/bin/time -v $BUILD_DIR/images/jdk/bin/java -Xmx256m -jar $BUILD_DIR/images/test/micro/benchmarks.jar LookupDef.*WeakClass.loadLong -f 0 -i N | grep "Maximum resident set"` yields:
Baseline: N = 20 Maximum resident set size (kbytes): 544860 N = 50 Maximum resident set size (kbytes): 818532 N = 100 Maximum resident set size (kbytes): 1388560 N = 200 Maximum resident set size (kbytes): 2124296 Patch: N = 20 Maximum resident set size (kbytes): 480476 N = 50 Maximum resident set size (kbytes): 764040 N = 100 Maximum resident set size (kbytes): 782920 N = 200 Maximum resident set size (kbytes): 921272
This pull request has now been integrated. Changeset: 07918995 Author: Claes Redestad <redestad@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/07918995 Stats: 79 lines in 2 files changed: 79 ins; 0 del; 0 mod 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names Reviewed-by: stuefe, chegar, mchung ------------- PR: https://git.openjdk.java.net/jdk/pull/2407
participants (4)
-
Chris Hegarty
-
Claes Redestad
-
Mandy Chung
-
Thomas Stuefe