February 2021 - core-libs-dev

Re: RFR: 8261449: Micro-optimize JVM_LatestUserDefinedLoader
by David Holmes 10 Feb '21

10 Feb '21

On Tue, 9 Feb 2021 15:40:03 GMT, Aleksey Shipilev <shade(a)openjdk.org> wrote: > `JVM_LatestUserDefinedLoader` is called normally from `ObjectInputStream.resolveClass` -> `VM.latestUserDefinedLoader0`. And it takes a measurable time to walk the stack. There is JDK-8173368 that wants to replace it with `StackWalker`, but we can tune up the `JVM_LatestUserDefinedLoader` itself without changing the semantics of it (thus providing the backportability, including the releases that do not have `StackWalker`) and improving performance (thus providing a more aggressive baseline for `StackWalker` rewrite). > > The key is to recognize that out of two checks: 1) checking for two special subclasses; 2) checking for user classloader -- the first one usually passes, and second one fails much more frequently. First check also requires traversing the superclasses upwards looking for match. Reversing the order of the checks, plus inlining the helper method improves performance without changing the semantics. > > Out of curiosity, my previous patch dropped the first check completely, replacing it by asserts, and we definitely run into situation where that check is needed on some tests. > > On my machine, `VM.latestUserDefinedLoader` invocation time drops from 115 to 100 ns/op. Single-threaded SPECjvm2008:serial improves about 3% with this patch. > > Additional testing: > - [x] Ad-hoc benchmarks > - [x] Linux x86_64 fastdebug, `tier1`, `tier2`, `tier3` > > --------- > ### Progress > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change must be properly reviewed > > > > ### Download > `$ git fetch https://git.openjdk.java.net/jdk pull/2485/head:pull/2485` > `$ git checkout pull/2485` Hi Aleksey, This seems reasonable to me. The generated reflection classes are loaded by a temporary loader (so they can be unloaded) and so have to be skipped. I've added core-libs to the PR as this is the VM side of their code and I want to make sure nothing has been overlooked. Thanks, David src/hotspot/share/prims/jvm.cpp line 3293: > 3291: oop loader = ik->class_loader(); > 3292: if (loader != NULL && !SystemDictionary::is_platform_class_loader(loader)) { > 3293: if (!ik->is_subclass_of(vmClasses::reflect_MethodAccessorImpl_klass()) && Please add a comment: // Skip reflection related frames ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2485

2 1

Jpackage GraalVM OS X
by Michael Hall 09 Feb '21

09 Feb '21

This appears to no longer work at all as a included JRE With… --runtime-image /Library/Java/JavaVirtualMachines/graalvm-ce-java11-21.0.0/Contents/Home \ It fails to launch. Launch CLI outFastR/FastRGraalHP.app/Contents/MacOS/FastRGraalHP Failed to find JVM in "/Users/mjh/HalfPipe/HalfPipe_jpkg/outFastR/FastRGraalHP.app/Contents/runtime" directory. Although it is in fact there… ls /Users/mjh/HalfPipe/HalfPipe_jpkg/outFastR/FastRGraalHP.app/Contents/runtime/Contents Home Info.plist MacOS _CodeSignature I thought code signing might fix some other issues I had with this previously now that that is working.

1 0

RFR: 8223188: Consider rewriting in C++ or moving to ".c" files
by Alexey Semenyuk 09 Feb '21

09 Feb '21

Remove needless `#ifdef __cplusplus` from .cpp sources ------------- Commit messages: - 8223188: Consider rewriting in C++ or moving to ".c" files Changes: https://git.openjdk.java.net/jdk/pull/2466/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2466&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8223188 Stats: 10 lines in 2 files changed: 0 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2466.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2466/head:pull/2466 PR: https://git.openjdk.java.net/jdk/pull/2466

5 9

RFR: 8261209: isStandalone property: remove dependency on pretty-print
by Joe Wang 09 Feb '21

09 Feb '21

A quick fix to remove isStandalone's dependency on pretty-print. ------------- Commit messages: - 8261209: isStandalone property: remove dependency on pretty-print Changes: https://git.openjdk.java.net/jdk/pull/2442/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2442&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261209 Stats: 36 lines in 3 files changed: 18 ins; 2 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/2442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2442/head:pull/2442 PR: https://git.openjdk.java.net/jdk/pull/2442

3 3

JEP draft: Use UTF-8 as default Charset
by Naoto Sato 09 Feb '21

09 Feb '21

Hello, We are planning to implement the following JEP draft: https://openjdk.java.net/jeps/8187041 This is a long-standing issue that has been causing cross-platform default encoding problems. Now that UTF-8 has increasingly become the de-facto character set out there, we believe it is high time to make UTF-8 as the default charset in the JDK. More detailed description can be found in the above JEP draft, so please take a look at it, and let us know your thoughts. Thank you. Naoto

1 0

RFR: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
by Aleksey Shipilev 09 Feb '21

09 Feb '21

SonarCloud instance reports a new warning after JDK-8254702: "Use of memory after it is freed" char* getJvmLauncherLibPath(void) { ... popenStatus = popenCommand(pkgQueryCmd, pkg->name, findLauncherLib, &launcherLibPath); if (popenStatus) { free(launcherLibPath); <---- frees here goto cleanup; } } cleanup: free(modulePath); freePackageDesc(pkg); return launcherLibPath; <--- here } We need to null it out before returning. Additional testing: - [x] Linux x86_64 (Ubuntu) `tools/jpackage` ------------- Commit messages: - Null it out Changes: https://git.openjdk.java.net/jdk/pull/2453/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2453&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261299 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2453.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2453/head:pull/2453 PR: https://git.openjdk.java.net/jdk/pull/2453

3 6

RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
by Naoto Sato 09 Feb '21

09 Feb '21

Please review this simple test case fix. By sampling locales to test, it reduces the possibility of the time out. ------------- Commit messages: - 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out Changes: https://git.openjdk.java.net/jdk/pull/2465/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2465&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261279 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2465.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2465/head:pull/2465 PR: https://git.openjdk.java.net/jdk/pull/2465

4 7

RFR: 8261306: ServiceLoader documentation has malformed Unicode escape
by Brian Burkhalter 09 Feb '21

09 Feb '21

Please review this really small correction to the class level documentation of `java.util.ServiceLoader`. ------------- Commit messages: - 8261306: ServiceLoader documentation has malformed Unicode escape Changes: https://git.openjdk.java.net/jdk/pull/2468/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2468&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261306 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2468.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2468/head:pull/2468 PR: https://git.openjdk.java.net/jdk/pull/2468

4 4

RFR: 8198540: Dynalink leaks memory when generating type converters
by Attila Szegedi 09 Feb '21

09 Feb '21

_(NB: For this leak to occur, an application needs to be either creating and discarding linkers frequently, or creating and discarding class loaders frequently while creating Dynalink type converters for classes in them. Neither is typical usage, although they can occur in dynamic code loading environments such as OSGi.)_ I'll explain this one in a bit more detail. Dynalink's creates and caches method handles for language-specific conversions between types. Each conversion is thus characterized by a `Class` object representing the type converted from, and a `Class` object representing the type converted to, thus they're keyed by a pair of `(Class, Class)`. Java API provides the excellent `ClassValue` class for associating values with a single class, but it lacks the – admittedly more niche – facility for associating a value with a pair of classes. I originally solved this problem using something that looks like a `ClassValue<Map<Class<?>, T>>`. I say "looks like" because Dynalink has a specialized `ClassMap` class that works like `Map<Class<?>, T>` but it also goes to some pains to _not_ establish strong references from a class loader to either its children or to unrelated class loaders, for obvious reasons. Surprisingly, the problem didn't lie in there, though. The problem was in the fact that `TypeConverterFactory` used `ClassValue` objects that were its non-static anonymous inner classes, and further the values they computed were also of non-static anonymous inner classes. Due to the way `ClassValue` internals work, this led to the `TypeConverterFactory` objects becoming anchored in a GC root of `Class.classValueMap` through the synthetic `this$0` reference chains in said inner classes… talk about non-obvious memory leaks. (I guess there's a lesson in there about not using `ClassValue` as an instance field, but there's a genuine need for them here, so…) … so the first thing I did was I deconstructed all those anonymous inner classes into named static inner classes, and ended up with something that _did_ fix the memory leak (yay!) but at the same time there was a bunch of copying of constructor parameters from outer classes into the inner classes, the whole thing was very clunky with scary amounts of boilerplate. I felt there must exist a better solution. And it exists! I ended up replacing the `ClassValue<ClassMap<T>>` construct with a ground-up implementation of `BiClassValue<T>`, representation of lazily computed values associated with a pair of types. This was the right abstraction the `TypeConverterFactory` code needed all along. `BiClassValue<T>` is also not implemented as an abstract class but rather it is a final class and takes a `BiFunction<Class<?>, Class<?>, T>` for computation of its values, thus there's never a risk of making it an instance-specific inner class (well, you still need to be careful with the bifunction lambda…) It also has an even better solution for avoiding strong references to child class loaders. And that's why I'm not submitting here the smallest possible point fix to the memory leak, but rather a slightly larger one that architecturally fixes it the right way. There are two test classes, they test subtly different scenarios. `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory` instance becomes unreachable, all method handles it created also become eligible for collection. `TypeConverterFactoryRetentionTests` on the other hand test that the factory itself won't prevent class loaders from being collected by virtue of creating converters for them. ------------- Commit messages: - Create the right abstraction, BiClassValue and retire ClassMap. - Create a test Changes: https://git.openjdk.java.net/jdk/pull/1918/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8198540 Stats: 746 lines in 5 files changed: 512 ins; 220 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/1918.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1918/head:pull/1918 PR: https://git.openjdk.java.net/jdk/pull/1918

5 30

RFR: 8261096: Convert jlink tool to use Stream.toList()
by Ian Graves 09 Feb '21

09 Feb '21

A subtask [JDK-8260559](https://bugs.openjdk.java.net/browse/JDK-8260559). This is an enhancement to update jlink's usage of `Collections.toList()` to `Stream.toList()`. ------------- Commit messages: - Converting jlink usage of Collections.toList() to Stream.toList Changes: https://git.openjdk.java.net/jdk/pull/2416/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2416&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261096 Stats: 18 lines in 8 files changed: 0 ins; 5 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/2416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2416/head:pull/2416 PR: https://git.openjdk.java.net/jdk/pull/2416

3 4