Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Thu, 1 Jun 2023 19:39:27 GMT, Mandy Chung wrote: > As for the implementation of `LazyStaticVarHandle`, I expect that > `makeFieldHandle` can always create the direct var handle (i.e. > `FieldStaticReadXXX` var handle) and then wrapped by `LazyStaticVarHandle` > similar to how `IndirectVarHandle` does; Since I am not familiar with hotspot internals, I assumed that staticFieldBase and staticFieldOffset shouldn't be used until the class is initialized (rather than linked). This for certain can be done if they are ready once a class is linked, and we should probably update unsafe's comment as well. Can you confirm that they staticFieldBase/Offset are safe to be called before initialization? If this change is safe, we can significantly reduce the initial-call overhead of the lazy VH significantly as well. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572801392
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported I think the exact argument for `withInvokeExactBehavior` and `withInvokeBehavior` is swapped. I studied the patch and it seems a clever idea. I will do another pass to check if `VarHandle::asDirect` is only invoked when the var handle is doing field access. As for the implementation of `LazyStaticVarHandle`, I expect that `makeFieldHandle` can always create the direct var handle (i.e. `FieldStaticReadXXX` var handle) and then wrapped by `LazyStaticVarHandle` similar to how `IndirectVarHandle` does; something like this: LazyStaticVarHandle(VarHandle target, Class refc, boolean exact) { super(target.vform, exact); this.target = target; this.refc = refc; } Some overridden methods for example `describeConstable` can simply retrun `target.describeConstable()`. Do I miss some issue that it can't be implemented that way? src/java.base/share/classes/java/lang/invoke/LazyStaticVarHandle.java line 126: > 124: var delegate = this.delegate; > 125: return delegate == null ? (hasInvokeExactBehavior() ? this > 126: : new LazyStaticVarHandle(refc, field, > writeAllowedOnFinalFields, false)) Suggestion: : new LazyStaticVarHandle(refc, field, writeAllowedOnFinalFields, true)) src/java.base/share/classes/java/lang/invoke/LazyStaticVarHandle.java line 134: > 132: var delegate = this.delegate; > 133: return delegate == null ? (!hasInvokeExactBehavior() ? this > 134: : new LazyStaticVarHandle(refc, field, > writeAllowedOnFinalFields, true)) Suggestion: : new LazyStaticVarHandle(refc, field, writeAllowedOnFinalFields, false)) - PR Review: https://git.openjdk.org/jdk/pull/13821#pullrequestreview-1456188245 PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572667645 PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1213602428 PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1213602313
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported src/java.base/share/classes/java/lang/invoke/VarHandles.java line 111: > 109: else { > 110: if (UNSAFE.shouldBeInitialized(refc)) { > 111: return new LazyStaticVarHandle(refc, f, > isWriteAllowedOnFinalFields, false); This should probably call `maybeAdapt` on the result: Suggestion: return maybeAdapt(new LazyStaticVarHandle(refc, f, isWriteAllowedOnFinalFields, false)); - PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1207261501
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported I'm aware of these PRs. I'll get to it when I get to it. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1564639138
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported @mlchung Would you mind taking a look at this? - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1564386002
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. > > A few implementation-detail methods in VarHandle are now documented with the > implied constraints to avoid subtle problems in the future. Changed > `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy > VarHandle changes. Also changed VarHandleBaseTest to report the whole > incorrect type of exception caught than swallow it and leaving only a message. > > Current problems: > - [ ] The lazy var handle is quite slow on the first invocation. >- As seen in the benchmark, users can first call > `Lookup::ensureInitialized` to create an eager handle. >- After that, the lazy handle has a performance on par with the regular > var handle. > - [ ] The class-loading-based test is not in a unit test >- The test frameworks don't seem to offer fine-grained control for > class-loading detection or reliable unloading > > > BenchmarkMode Cnt Score > Error Units > VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± > 0.003 ns/op > VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± > 0.002 ns/op > > > BenchmarkMode Cnt Score > Error Units > LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± > 7735.276 ns/op > LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± > 8482.883 ns/op > LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± > 9776.343 ns/op > LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± > 72894.865 ns/op > LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± > 3513.909 ns/op > LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± > 5144.540 ns/op > LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± > 10816.983 ns/op > LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± > 189370.448 ns/op Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Remove export for removed package - Merge branch 'master' into lazy-static-varhandle - Test the creation performance of handles, lazy one indeed better - Merge branch 'master' into lazy-static-varhandle - copyright year - Benchmarks. lazy initialize is SLOW - nuke meaningless overrides - make static field var handles lazy and fix NPE in isAccessModeSupported - Changes: - all: https://git.openjdk.org/jdk/pull/13821/files - new: https://git.openjdk.org/jdk/pull/13821/files/eea7aa84..6ed6b805 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13821&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13821&range=01-02 Stats: 10143 lines in 473 files changed: 5357 ins; 1503 del; 3283 mod Patch: https://git.openjdk.org/jdk/pull/13821.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13821/head:pull/13821 PR: https://git.openjdk.org/jdk/pull/13821