January 2021 - core-libs-dev

RFR: 8260401: StackOverflowError on open WindowsPreferences
by Jaikiran Pai 13 Feb '21

13 Feb '21

Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8260401? As noted in that issue, when the constructor of `java.util.prefs.WindowsPreferences` runs into an error while dealing with the Windows registry, it logs a warning message. The message construction calls `rootNativeHandle()` (on the same instance of `WindowsPreferences` that's being constructed) which then ultimately ends up calling the constructor of `WindowsPreferences` which then again runs into the Windows registry error and attempts to log a message and this whole cycle repeats indefinitely leading to that `StackOverFlowError`. Based on my limited knowledge of the code in that constructor and analyzing that code a bit, it's my understanding (and also the input provided by the reporter of the bug) that the log message should actually be using the `rootNativeHandle` parameter that is passed to this constructor instead of invoking the `rootNativeHandle()` method. The commit in this PR does this change to use the passed `rootNativeHandle` in the log message. Furthermore, there's another constructor in this class which does a similar thing and potentially can run into the same error as this one. I've changed that constructor too, to avoid the call to `rootNativeHandle()` method in that log message. Reading the log message that's being generated there, it's my understanding that this change shouldn't cause any inaccuracy in what's being logged and in fact, I think it's now more precise about which handle returned the error code. Finally, this log message creation, in both the constructors, also calls `windowsAbsolutePath()` and `byteArrayToString()` methods. The `byteArrayToString()` is a static method and a call to it doesn't lead back to the constructor again. The `windowsAbsolutePath()` is a instance method and although it does use a instance variable `absolutePath`, that instance variable is already initialized appropriately by the time this `windowsAbsolutePath()` gets called in the log message. Also, the `windowsAbsolutePath()` call doesn't lead back to the constructor of the `WindowsPreferences` so it doesn't pose the same issue as a call to `rootNativeHandle()`. Given the nature of this issue, I haven't added any jtreg test for this change. ------------- Commit messages: - 8260401: StackOverflowError on open WindowsPreferences Changes: https://git.openjdk.java.net/jdk/pull/2326/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2326&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260401 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2326.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2326/head:pull/2326 PR: https://git.openjdk.java.net/jdk/pull/2326

2 11

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: 8260337: Optimize ImageReader lookup, used by Class.getResource
by Claes Redestad 09 Feb '21

09 Feb '21

This patch optimizes the code paths exercised by `String.class.getResource("String.class")` by: - Adding an ASCII fast-path to methods verifying strings in the jimage, which can then be done allocation-free - Avoiding the allocation of the `long[8]` attributes when verifying only for the purpose of verifying a path exists - Using the `JNUA.create` fast-path in `SystemModuleReader` (which should be OK since we just verified the given name is a JRT path) - Remove a redundant check in `Class::resolveName` and fitting the `StringBuilder` to size ------------- Commit messages: - Remove more redundant checks - More cleanups, remove redundant null checks, synchronization - Minor cleanup, remove some code duplication - Fix offsets to properly find extension-less resources (ModuleReaderTest etc) - Merge branch 'master' into jrt_getString - Make ImageLocation lookup mostly allocation-free, use JNUA.create in SystemModuleReader, improve Class.resolveName - Invert length for non-ASCII case - Add working micro, cleanup and fix code - Improve jimage string reading Changes: https://git.openjdk.java.net/jdk/pull/2212/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2212&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260337 Stats: 387 lines in 8 files changed: 263 ins; 63 del; 61 mod Patch: https://git.openjdk.java.net/jdk/pull/2212.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2212/head:pull/2212 PR: https://git.openjdk.java.net/jdk/pull/2212

5 12

RFR: 8259074: regex benchmarks and tests
by Martin Buchholz 08 Feb '21

08 Feb '21

8259074: regex benchmarks and tests ------------- Commit messages: - more benchmarks - JDK-8259074 Changes: https://git.openjdk.java.net/jdk/pull/1940/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1940&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259074 Stats: 444 lines in 5 files changed: 442 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1940.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1940/head:pull/1940 PR: https://git.openjdk.java.net/jdk/pull/1940

5 19

RFR: 8166026: refactor shell tests to java
by Ivan Šipka 06 Feb '21

06 Feb '21

@iignatev could you please review? Thank you. note to self: jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java ------------- Commit messages: - 8166026: removing tab character - 8166026: refactor shell tests to java Changes: https://git.openjdk.java.net/jdk/pull/1484/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1484&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8166026 Stats: 1002 lines in 12 files changed: 496 ins; 505 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1484/head:pull/1484 PR: https://git.openjdk.java.net/jdk/pull/1484

3 14

Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
by Alexey Semenyuk 05 Feb '21

05 Feb '21

> Fix for https://bugs.openjdk.java.net/browse/JDK-8254702 > > The fix splits Linux app launcher in app launcher and launcher shared lib. App launcher is pure C and doesn't have C++ code. App launcher lib incorporates bulk of C++ code from app launcher. > At startup app launcher loads launcher shared lib and calls functions it provides to get data for launching JVM (path to jli lib and arguments for JLI_Launch function call). > App launcher unloads launcher shared lib before launching JVM to remove C++ runtime from the process memory. > > Getting rid of C++ code from app launcher required to rewrite app installation location lookup code from C++ to C. LinuxPackage.c source is C alternative for https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/ap… and https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/ap…. > > Layout of jpackage's native code changed: > - `common`: code shared between all jpackage binaries. > - `applauncher`: launcher only code. > - `applauncherlib`: launcher lib code on Linux and launcher code on other platforms. > - `applaunchercommon`: code shared between launcher and launcher lib on Linux and launcher code on other platforms. Alexey Semenyuk has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8254702: jpackage app launcher crashes on CentOS ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2320/files - new: https://git.openjdk.java.net/jdk/pull/2320/files/14d277a2..b493bcfd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2320&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2320&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2320/head:pull/2320 PR: https://git.openjdk.java.net/jdk/pull/2320

5 8

RFR: JDK-8260335: [macos] Running app using relative path causes problems
by Andy Herrick 04 Feb '21

04 Feb '21

Fixing FileUtils.dirname() to skip over "/.". ------------- Commit messages: - JDK-8260335: [macos] Running app using relative path causes problems on MacOS Changes: https://git.openjdk.java.net/jdk/pull/2260/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2260&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260335 Stats: 10 lines in 1 file changed: 8 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2260.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2260/head:pull/2260 PR: https://git.openjdk.java.net/jdk/pull/2260

4 6

RFR: 8166026: Refactor java/lang shell tests to java
by Ivan Šipka 03 Feb '21

03 Feb '21

Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test. ------------- Commit messages: - 8166026: Refactor java/lang shell tests to java Changes: https://git.openjdk.java.net/jdk/pull/1578/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1578&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8166026 Stats: 411 lines in 2 files changed: 206 ins; 205 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1578/head:pull/1578 PR: https://git.openjdk.java.net/jdk/pull/1578

2 11

RFR: 8260617: Merge ZipFile encoding check with the initial hash calculation
by Claes Redestad 03 Feb '21

03 Feb '21

- Merge checkEncoding into the byte[]-based normalizedHash. The latter is only used from ZipFile.initCEN right after the checkEncoding today, so verifying this is equivalent is straightforward. - Factor out the logic to calculate hash, check encoding etc into the addEntry method to allow JITs to chew off larger chunks of the logic early on Roughly 1.2x speedup on the JarFileMeta microbenchmarks, which include the time required to open the jar file and thus exercising the optimized code. Testing: tier1-4 ------------- Commit messages: - Refactor ZipFile.initCEN, merging checkEncoding and normalizedHash Changes: https://git.openjdk.java.net/jdk/pull/2306/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2306&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260617 Stats: 132 lines in 2 files changed: 31 ins; 68 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/2306.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2306/head:pull/2306 PR: https://git.openjdk.java.net/jdk/pull/2306

2 11

Trying to fix JDK-8013527 - 1st Prototype
by Johannes Kuhn 03 Feb '21

03 Feb '21

JDK-8013527[1] has somehow become the umbrella bug for "Using MethodHandles to call caller sensitive methods leads to interesting results". To recap: A caller sensitive method knows about who called it, and can behave differently when called from an other context. Examples are: Class.forName, MethodHandles.lookup, Method.invoke... A MethodHandle on the other hand should not be caller sensitive. To archive this, a MethodHandle will "bind" the lookup class as caller for caller sensitive methods. This is currently done by injecting a hidden class (InjectedInvoker" that acts as a trampoline for calling caller sensitive methods. This injected invoker shares many properties of the original caller: Same ClassLoader, same Module, same Package, same ProtectionDomain, but it's not the same class or a nestmate of it. For caller sensitive methods that do look at more than just the injected invoker, this leads to "unexpected" results when called through a MethodHandle: * MethodHandles.lookup() returns a full privileged lookup for the injected invoker. * jlr.Field.get*/set*, jlr.Constructor.newInstance, jlr.Method.invoke may fail with an IllegalAccessException if the target is private. See JDK-8257874[2]. ----------------------------------- After reading one of John Rose's comments[3], I thought that this might be a way to solve this general problem. So I implemented some of it here[4]. The basic idea is that there is a private overload of the caller sensitive method which accepts the caller as a tailing argument. The good news: * tier1 Tests pass. * ((Lookup) lookup.findStatic(MethodHandles.class, "lookup", MethodType.methodType(Lookup.class)).invokeExact()).lookupClass() == lookup.lookupClass(); * JDK-8257874 can't be reproduced with Field.* or Constructor. * Performance is likely better. (InjectedInvoker collects all arguments into an Object[].) The bad news: * If you use a MethodHandle to call Method.invoke for a caller sensitive method, then you can still observe the injected invoker. ----------------------------------- Moving forward, there are 3 ways: 1. Do nothing. Won't fix any bug. 2. Use the current prototype, and accept Method.invoke is odd when calling it through a MethodHandle. 3. Go all in: * Require **every** caller sensitive method to have a private overload. * Method.invoke will also use that private overload. The problems with the 3rd approach are: * What about methods that can be called virtually? (Thread.getContextClassLoader()) * Requires a few changes to MethodAccessor. Maybe implementing JDK-6824466[5] first? * What about methods that do stack walks? I have to think more about the problems listed above - but maybe you have some input that could help me on that. - Johannes [1]: https://bugs.openjdk.java.net/browse/JDK-8013527 [2]: https://bugs.openjdk.java.net/browse/JDK-8257874 [3]: https://bugs.openjdk.java.net/browse/JDK-8020968?focusedCommentId=13611844&… [4]: https://github.com/openjdk/jdk/pull/2117/files [5]: https://bugs.openjdk.java.net/browse/JDK-6824466

2 1