May 2021 - core-libs-dev

Re: JEP draft: Scope Locals
by Andrew Haley 15 May '21

15 May '21

On 5/14/21 8:19 PM, Mike Rettig wrote: > I don't see a way to capture the lifecycle of the scoped variable. I think > for framework designers it's going to be important to know the life cycle > of the scoped variable especially with snapshotting and inheritance. The > lack of a defined life cycle for thread locals is one of the many reasons > why it should be avoided. This requirement sounds similar, but I think it's really a different idea. Scope locals are only binding data to names and don't attempt to handle lifetime issues. Apart from anything else, that would require a more heavyweight mechanism than scope locals, which need to be very lightweight for some applications. Over at Loom we've been talking about lifetimes and Ron has some ideas, but scope locals are not likely to be that thing. -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

1 0

Re: JEP draft: Scope Locals
by Andrew Haley 15 May '21

15 May '21

On 5/15/21 12:28 AM, Nathan Reynolds wrote: > What about cleanup of the scoped objects? For example, how do I ensure > close() or some cleanup method is called at the right time on the scoped > object? Do I need to use a Weak/Soft/PhantomReference and a ReferenceQueue > to track the object? Do I use Cleaner? Does the object need to implement > finalize()... hopefully not? All a scope local does is bind a value to a name, in a scope. This isn't about C++-style destructors, which have their merits but are a different subject. -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

1 0

RFR: 8249395: (macos) jpackage tests timeout on MacPro5_1 systems
by Alexander Matveev 14 May '21

14 May '21

Looks like it is similar to JDK-8236282, except for "hdiutil create". Based on call stack from failed test we launched "hdiutil create" and were waiting for it to terminate while reading output from it. However, based on process dump from machine which reproduced this issue, hdiutil no longer runs and existed long time ago. Fixed in same way as JDK-8236282 by writing output to file and then reading it back when process terminates. ------------- Commit messages: - 8249395: (macos) jpackage tests timeout on MacPro5_1 systems Changes: https://git.openjdk.java.net/jdk/pull/4021/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4021&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8249395 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4021.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4021/head:pull/4021 PR: https://git.openjdk.java.net/jdk/pull/4021

3 3

Re: JEP draft: Scope Locals
by Brian Goetz 14 May '21

14 May '21

The lifecycle is bounded by the duration of the Runnable passed to the `run()` method. In the simple case, with no inheritance and no snapshotting: ScopedLocal.where(x, xExpr).run(r) the lifecycle of the binding starts when we enter r, and ends when we return from r. With snapshotting, the answer is the same, just less obviously so. If I take a snapshot of a set of variables, we're not in a scope yet; we'll enter a new scope when we use that snapshot to run a task, possibly multiple times. With inheritance, it is possible that the child thread can live longer than the lifetime of the task that spawned it; this is something we hope we can avoid through _structured concurrency_ -- that the inheritance is restricted to cases where we can prove the lifetime of the parent is longer than the lifetime of the child. There's more work to be done here. To be explicit, the two assumptions we need in order to avoid people shooting their feet are: - ScopeLocal values are effectively final "all the way down". We can't enforce this, but if users had to synchronize when accessing the state of scoped locals, then someone made a big mistake. (This joins a very long list of mutability bugs we can't prevent but for which we have to communicate to users what they shouldn't be doing, not unlike "don't modify the source of the stream while traversing it.") - When inheriting, the parent must outlive the child. When these assumptions hold, understanding the lifecycle is trivial. On 5/14/2021 3:19 PM, Mike Rettig wrote: > I don't see a way to capture the lifecycle of the scoped variable. I think > for framework designers it's going to be important to know the life cycle > of the scoped variable especially with snapshotting and inheritance. The > lack of a defined life cycle for thread locals is one of the many reasons > why it should be avoided. If scoped locals have a well defined and > comprehensive life cycle then it will make working with them much easier > (especially when many threads are involved). > > interface ScopedVariableLifecycle<T> { > ScopedVariable<T> begin(); > } > > interface ScopedVariable<T> { > T get(); > void end(); > > Optional<ScopedVariableLifecycle<T>> createForChildScope(); > Optional<ScopedVariableLifecycle<T>> createSnapshot(); > } > > static final ScopeLocal<MyType> x = ScopeLocal.forType(MyType.class); > > //most scoped variables will probably use the standard singleton > implementations. > > ScopedLocal.where(x, new Singleton(new MyType())); > > ScopedLocal.where(x, new InheritableSingleton(new MyType())); > > > //a custom lifecycle (e.g. database connection that is opened on first > access and closed at the end of the scope). > > ScopedLocal.where(x, new MyScopedVariableLifecycle()); > > > On Wed, May 12, 2021 at 10:15 AM Andrew Haley <aph(a)redhat.com> wrote: > >> There's been considerable discussion about scope locals on the loom-dev >> list, >> and it's now time to open this to a wider audience. This subject is >> important >> because. although scope locals were motivated by the the needs of Loom, >> they >> have many potential applications outside that project. >> >> The draft JEP is at >> >> https://bugs.openjdk.java.net/browse/JDK-8263012 >> >> I've already received some very helpful suggestions for enhancements to >> the API, and it'll take me a while to work through them all. In particular, >> Paul Sandoz has suggested that I unify the classes Snapshot and Carrier, >> and it will take me some time to understand the consequences of that. >> >> In the meantime, please have a look at the JEP and comment here. >> >> >> For reference, earlier discussions are at >> >> https://mail.openjdk.java.net/pipermail/loom-dev/2021-March/002268.html >> https://mail.openjdk.java.net/pipermail/loom-dev/2021-April/002287.html >> https://mail.openjdk.java.net/pipermail/loom-dev/2021-May/002427.html >> >> -- >> Andrew Haley (he/him) >> Java Platform Lead Engineer >> Red Hat UK Ltd. <https://www.redhat.com> >> https://keybase.io/andrewhaley >> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671 >> >> >>

1 0

RFR: 8254598: StringDedupTable should use OopStorage
by Kim Barrett 14 May '21

14 May '21

Please review this change to the String Deduplication facility. It is being changed to use OopStorage to hold weak references to relevant objects, rather than bespoke data structures requiring dedicated processing phases by supporting GCs. (The Shenandoah update was contributed by Zhengyu Gu.) This change significantly simplifies the interface between a given GC and the String Deduplication facility, which should make it much easier for other GCs to opt in. However, this change does not alter the set of GCs providing support; currently only G1 and Shenandoah support string deduplication. Adding support by other GCs will be in followup RFEs. Reviewing via the diffs might not be all that useful for some parts, as several files have been essentially completely replaced, and a number of files have been added or eliminated. The full webrev might be a better place to look. The major changes are in gc/shared/stringdedup/* and in the supporting collectors, but there are also some smaller changes in other places, most notably classfile/{stringTable,javaClasses}. This change is additionally labeled for review by core-libs, although there are no source changes there. This change injects a byte field of bits into java.lang.String, using one of the previously unused padding bytes in that class. (There were two unused bytes, now only one.) Testing: mach5 tier1-2 with and without -XX:+UseStringDeduplication Locally (linux-x64) ran all of the existing tests that use string deduplication with both G1 and Shenandoah. Note that TestStringDeduplicationInterned.java is disabled for shenandoah, as it currently fails, for reasons I haven't figured out but suspect are test related. - gc/stringdedup/ -- these used to be in gc/g1 - runtime/cds/SharedStringsDedup.java - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java - runtime/cds/appcds/sharedStrings/* shenandoah-only: - gc/shenandoah/jvmti/TestHeapDump.java - gc/shenandoah/TestStringDedup.java - gc/shenandoah/TestStringDedupStress.java Performance tested baseline, baseline + stringdedup, new with stringdedup, finding no significant differences. ------------- Commit messages: - New string deduplication Changes: https://git.openjdk.java.net/jdk/pull/3662/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254598 Stats: 6185 lines in 105 files changed: 2361 ins; 3202 del; 622 mod Patch: https://git.openjdk.java.net/jdk/pull/3662.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662 PR: https://git.openjdk.java.net/jdk/pull/3662

6 27

Integrated: 8267180: Typo in copyright header for HashesTest
by Lance Andersen 14 May '21

14 May '21

Please review this fix for a typo in the copyright header in HashTest which the jdk-tier1 does not catch Best Lance ------------- Commit messages: - Typo in copyright header for HashesTest Changes: https://git.openjdk.java.net/jdk/pull/4034/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4034&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267180 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4034.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4034/head:pull/4034 PR: https://git.openjdk.java.net/jdk/pull/4034

4 4

RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
by Lance Andersen 14 May '21

14 May '21

HI all, Please review the fix to HashesTest.java to support running on TestNG 7.4. The fix adds a no-arg constructor which TestNG requires. The change allows the test to run with the current jtreg as well as the upcoming jtreg Best Lance ------------- Commit messages: - Remove extrawhitespace - Update HashesTest for TestNG 7.4 Changes: https://git.openjdk.java.net/jdk/pull/4009/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4009&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266461 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009

4 17

RFR: 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem
by Athijegannathan Sundararajan 14 May '21

14 May '21

Problematic paths that start with /modules/modules prefix are handled properly ------------- Commit messages: - fixed typos in comments - 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem Changes: https://git.openjdk.java.net/jdk/pull/4022/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4022&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266291 Stats: 36 lines in 2 files changed: 32 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4022.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4022/head:pull/4022 PR: https://git.openjdk.java.net/jdk/pull/4022

3 4

RFR: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0()
by Сергей Цыпанов 14 May '21

14 May '21

Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 it appears, that in `j.l.Class` expressions like String str = baseName.replace('.', '/') + '/' + name; are not compiled into invokedynamic-based code, but into one using `StringBuilder`. This happens due to some bootstraping issues. Currently the bytecode for the last (most often used) branch of `Class.descriptorString()` looks like public sb()Ljava/lang/String; L0 LINENUMBER 21 L0 NEW java/lang/StringBuilder DUP INVOKESPECIAL java/lang/StringBuilder.<init> ()V ASTORE 1 L1 LINENUMBER 23 L1 ALOAD 1 LDC "a" INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder; POP L2 LINENUMBER 24 L2 ALOAD 1 LDC "b" INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder; POP L3 LINENUMBER 26 L3 ALOAD 1 INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String; ARETURN Here the `StringBuilder` is created with default constructor and then expands if necessary while appending. This can be improved by manually allocating `StringBuilder` of exact size. The benchmark demonstrates measurable improvement: @State(Scope.Benchmark) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.NANOSECONDS) @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"}) public class ClassDescriptorStringBenchmark { private final Class<?> clazzWithShortDescriptor = Object.class; private final Class<?> clazzWithLongDescriptor = getClass(); @Benchmark public String descriptorString_short() { return clazzWithShortDescriptor.descriptorString(); } @Benchmark public String descriptorString_long() { return clazzWithLongDescriptor.descriptorString(); } } original -Xint Mode Score Error Units descriptorString_long avgt 6326.478 ± 107.251 ns/op descriptorString_short avgt 5220.729 ± 103.545 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 528.089 ± 0.021 B/op descriptorString_short:·gc.alloc.rate.norm avgt 232.036 ± 0.015 B/op -XX:TieredStopAtLevel=1 Mode Score Error Units descriptorString_long avgt 230.223 ± 1.254 ns/op descriptorString_short avgt 164.255 ± 0.755 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 528.046 ± 0.002 B/op descriptorString_short:·gc.alloc.rate.norm avgt 232.022 ± 0.001 B/op full Mode Score Error Units descriptorString_long avgt 74.835 ± 0.262 ns/op descriptorString_short avgt 43.822 ± 0.788 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 504.010 ± 0.001 B/op descriptorString_short:·gc.alloc.rate.norm avgt 208.004 ± 0.001 B/op ------------------------ patched -Xint Mode Score Error Units descriptorString_long avgt 4485.994 ± 60.173 ns/op descriptorString_short avgt 3949.965 ± 278.143 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 336.051 ± 0.004 B/op descriptorString_short:·gc.alloc.rate.norm avgt 184.027 ± 0.010 B/op -XX:TieredStopAtLevel=1 Mode Score Error Units descriptorString_long avgt 185.774 ± 1.100 ns/op descriptorString_short avgt 135.338 ± 1.066 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 336.030 ± 0.001 B/op descriptorString_short:·gc.alloc.rate.norm avgt 184.019 ± 0.001 B/op full Mode Score Error Units descriptorString_long avgt 42.864 ± 0.160 ns/op descriptorString_short avgt 27.255 ± 0.381 ns/op descriptorString_long:·gc.alloc.rate.norm avgt 224.005 ± 0.001 B/op descriptorString_short:·gc.alloc.rate.norm avgt 120.002 ± 0.001 B/op Same can be done also for Class.isHidden() branch in Class.descriptorString() and for Class.getCanonicalName0() ------------- Commit messages: - 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0() Changes: https://git.openjdk.java.net/jdk/pull/3903/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3903&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266622 Stats: 19 lines in 1 file changed: 15 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3903.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3903/head:pull/3903 PR: https://git.openjdk.java.net/jdk/pull/3903

3 10

RFR: 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2
by Ioi Lam 14 May '21

14 May '21

This bug was discovered during the development of [JDK-6824466](https://bugs.openjdk.java.net/browse/JDK-6824466): when CDS is enabled, if `BootLoader::loadClassOrNull` is called before `initPhase2`, it would trigger the initialization of the archived module graph in the wrong order. Because of unanticipated nesting of `<clinit>` methods, `BootLoader::SERVICES_CATALOG` becomes empty, causing future `ServiceLoader` operations to fail. The fix has 2 parts: - `BootLoader::loadClassOrNull` no longer calls `ClassLoaders::bootLoader()`. This avoids triggering the archived module graph initialization. Instead, it makes a direct call to `Classloader::findBootstrapClassOrNull()`. We don't actually need a `ClassLoader` instance for this call, so I changed `Classloader::findBootstrapClassOrNull()` to be a static method. - The object returned by `BootLoader::getServicesCatalog()` is now maintained inside `jdk.internal.loader.ClassLoaders`. Although not strictly required for the fix, this simplifies the initialization of the archived module graph. It also makes the logic consistent for the 3 built-in loaders (boot/platform/app). Testing: tiers1-4 in progress. I also verified that the bug reported by Mandy is no longer reproducible after I applied this patch on her branch. ------------- Commit messages: - 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2 Changes: https://git.openjdk.java.net/jdk/pull/3992/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3992&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265605 Stats: 42 lines in 8 files changed: 8 ins; 13 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/3992.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3992/head:pull/3992 PR: https://git.openjdk.java.net/jdk/pull/3992

3 13