On Wed, 28 Jun 2023 01:34:30 GMT, Chen Liang <li...@openjdk.org> wrote:
>> This patch implements lazy initialization for VarHandle working on static
>> fields. It has a good initial call performance.
>>
>> We introduce a new internal API, `target()` to unpack a lazy VarHandle in VH
>> implementation methods. If called via MethodHandle, a barrier is added in
>> the MethodHandle instead.
>>
>> The new test ensures the correctness of Lazy VH for both direct and indirect
>> invocation; the performance of MethodHandle version of lazy VH is not yet
>> tested.
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> LazyStaticColdStart.methodHandleCreateEager ss 10 41.490 ±
>> 12.331 us/op
>> LazyStaticColdStart.methodHandleCreateLazy ss 10 21.810 ±
>> 16.964 us/op
>> LazyStaticColdStart.methodHandleInitializeCallEager ss 10 57.860 ±
>> 13.738 us/op
>> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 93.300 ±
>> 18.858 us/op
>> LazyStaticColdStart.varHandleCreateEager ss 10 39.860 ±
>> 9.362 us/op
>> LazyStaticColdStart.varHandleCreateLazy ss 10 17.630 ±
>> 1.111 us/op
>> LazyStaticColdStart.varHandleInitializeCallEager ss 10 123.170 ±
>> 62.468 us/op
>> LazyStaticColdStart.varHandleInitializeCallLazy ss 10 105.390 ±
>> 41.815 us/op
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Comment update from mandy
>
> Co-authored-by: Mandy Chung <mandy.ch...@oracle.com>
Looks good in general. Some minor comments.
I suggest to create a CSR to document the behavioral change to match the
specification.
src/java.base/share/classes/java/lang/invoke/LazyInitializingVarHandle.java
line 129:
> 127:
> 128: try {
> 129: return MH_ensureInitialized =
> IMPL_LOOKUP.findVirtual(LazyInitializingVarHandle.class,
Suggestion:
return MH_ensureInitialized =
MethodHandles.lookup().findVirtual(LazyInitializingVarHandle.class,
No need to use the superpower lookup.
src/java.base/share/classes/java/lang/invoke/VarHandle.java line 2086:
> 2084: * be linked directly. If a VarHandle is indirect, it must override
> 2085: * {@link #isAccessModeSupported} and {@link
> #getMethodHandleUncached}
> 2086: * which access MemberNames..
Suggestion:
* which access MemberNames.
test/jdk/java/lang/invoke/VarHandles/LazyInitializingTest.java line 30:
> 28: * initialization mechanism..
> 29: * @run junit LazyInitializingTest
> 30: * @run junit/othervm
> -Djava.lang.invoke.VarHandle.VAR_HANDLE_IDENTITY_ADAPT=true
> LazyInitializingTest
I wonder if this new test should run with all combination of
`java.lang.invoke.VarHandle.VAR_HANDLE_GUARDS` and
`java.lang.invoke.VarHandle.VAR_HANDLE_IDENTITY_ADAPT` as done in other
`VarHandleTestXXX`.
-------------
Marked as reviewed by mchung (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13821#pullrequestreview-1504321038
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1245899498
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1245901726
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1245905901
