-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8160821: VarHandle accesses are penalized when argument conversion is required #28585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
522cbe9
886d391
7bcdcbf
b8039d6
7ca6a5c
fa80b4e
80237bc
d49ad12
89e21b4
dd76a64
ff7b362
8200fb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved. | ||
| * Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
|
|
@@ -2017,14 +2017,49 @@ static final class AccessDescriptor { | |
| final int type; | ||
| final int mode; | ||
|
|
||
| public AccessDescriptor(MethodType symbolicMethodType, int type, int mode) { | ||
| // Adaption mechanism to reduce overhead for non-exact access. | ||
| // This heuristic assumes that each sigpoly VH call site usually sees | ||
| // exactly one VarHandle instance. Each sigpoly VH call site already | ||
| // has a dedicated AccessDescriptor. | ||
| // (See MethodHandleNatives::varHandleOperationLinkerMethod) | ||
| // However, for correctness, we must verify the incoming VarHandle; | ||
| // adaptedMethodHandle may be inlined by different callers. | ||
| // In the long run, we wish to put a specific-type invoker that converts | ||
| // from one fixed type (symbolicMethodTypeInvoker) to another (the | ||
| // invocation type of the underlying MemberName, or MH for indirect VH), | ||
| // perform a foldable lookup with a hash table, and hope C2 inline it | ||
| // all. | ||
|
|
||
| // Object indirection is the only way to ensure the vh and mh are not | ||
| // from two writes (they must not be tearable) | ||
| private record Adaption(VarHandle vh, MethodHandle mh) {} | ||
| private @Stable Adaption adaption; | ||
|
|
||
| AccessDescriptor(MethodType symbolicMethodType, int type, int mode) { | ||
| this.symbolicMethodTypeExact = symbolicMethodType; | ||
| this.symbolicMethodTypeErased = symbolicMethodType.erase(); | ||
| this.symbolicMethodTypeInvoker = symbolicMethodType.insertParameterTypes(0, VarHandle.class); | ||
| this.returnType = symbolicMethodType.returnType(); | ||
| this.type = type; | ||
| this.mode = mode; | ||
| } | ||
|
|
||
| @ForceInline | ||
| MethodHandle adaptedMethodHandle(VarHandle vh) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate, please, how this method is intended to behave?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When this is compiled,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I should move the adaptedMh read into
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still have a hard time reasoning about state transitions of the cache.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am assuming that if C2 determines this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I still don't understand how it is intended to work. Why does Capturing happens outside compiler thread. It is not affected by C2 (except when it completely prunes the whole block). So, either any captured adaptation is valid/compatible or there's a concurrency issue when C2 kicks in and there's a concurrent cache update happening with incompatible version.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, if For thread safety, MethodHandle supports safe publication, so I think we are fine publishing this way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this, I'm not sure we can assume that we only see one mode and type when the VH is constant. There seems to be a lot of non-local reasoning involved. For example, you could have a var handle invoker created with The thread safety issue comes from a C2 thread racing to read the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think even without using an invoker, you could end up in a similar situation if you have something like: Which is called by several different threads. At some point this method may be inlined into one of its callees, where |
||
| var cache = adaption; | ||
| if (cache != null && cache.vh == vh) { | ||
| return cache.mh; | ||
| } | ||
|
|
||
| var mh = vh.getMethodHandle(mode).asType(symbolicMethodTypeInvoker); | ||
| if (cache == null) { | ||
| // Reduce costly object allocation - if our assumption stands, | ||
| // the first adaption works, and we don't want allocations for | ||
| // every VH invocation. | ||
| adaption = new Adaption(vh, mh); | ||
| } | ||
| return mh; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| package compiler.c2.irTests.constantFold; | ||
|
|
||
| import java.lang.invoke.MethodHandles; | ||
| import java.lang.invoke.VarHandle; | ||
|
|
||
| import compiler.lib.ir_framework.Check; | ||
| import compiler.lib.ir_framework.IR; | ||
| import compiler.lib.ir_framework.IRNode; | ||
| import compiler.lib.ir_framework.Run; | ||
| import compiler.lib.ir_framework.Test; | ||
| import compiler.lib.ir_framework.TestFramework; | ||
|
|
||
| /* | ||
| * @test | ||
| * @bug 8160821 | ||
| * @summary Verify constant folding is possible for mismatched VarHandle access | ||
| * @library /test/lib / | ||
| * @requires vm.compiler2.enabled | ||
| * @run driver compiler.c2.irTests.constantFold.VarHandleMismatchedTypeFold | ||
| */ | ||
| public class VarHandleMismatchedTypeFold { | ||
|
|
||
| public static void main(String[] args) { | ||
| TestFramework.runWithFlags( | ||
| "-XX:+UnlockExperimentalVMOptions" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this flag needed? |
||
| ); | ||
| } | ||
|
|
||
| static final int a = 5; | ||
|
|
||
| static final VarHandle vh; | ||
|
|
||
| static { | ||
| try { | ||
| vh = MethodHandles.lookup().findStaticVarHandle(VarHandleMismatchedTypeFold.class, | ||
| "a", int.class); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new ExceptionInInitializerError(e); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| @IR(failOn = {IRNode.ADD_L, IRNode.LOAD_L}) | ||
| public long testSum() { | ||
| return 2L + (long) vh.get(); | ||
| } | ||
|
|
||
| @Check(test = "testSum") | ||
| public void runTestSum() { | ||
| long sum = testSum(); | ||
| if (sum != 2L + 5L) { | ||
| throw new IllegalStateException("Failed, unexpected sum " + sum); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| /* | ||
| * @test | ||
| * @bug 8160821 | ||
| * @summary Ensures a polymorphic call site with non-exact invocation won't | ||
| * be incorrectly inlined/optimized | ||
| * @requires vm.compiler2.enabled | ||
| * @run main/othervm -XX:CompileCommand=PrintCompilation,Main::* | ||
| * -XX:CompileCommand=dontinline,Main::payload* | ||
| * -Xbatch | ||
| * PolymorphicCallSiteInlineTest | ||
| */ | ||
|
|
||
| import java.lang.invoke.*; | ||
| import java.util.concurrent.CountDownLatch; | ||
|
|
||
| public class PolymorphicCallSiteInlineTest { | ||
|
|
||
| // C2 should inline m into payload1/payload2 in this many runs | ||
| static final int RUNS = 0x10000; | ||
|
|
||
| static final int X = 0; | ||
| static final long Y = 0L; | ||
|
|
||
| static final VarHandle VH_X; | ||
| static final VarHandle VH_Y; | ||
|
|
||
| static { | ||
| try { | ||
| var lookup = MethodHandles.lookup(); | ||
| VH_X = lookup.findStaticVarHandle(PolymorphicCallSiteInlineTest.class, "X", int.class); | ||
| VH_Y = lookup.findStaticVarHandle(PolymorphicCallSiteInlineTest.class, "Y", long.class); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new ExceptionInInitializerError(e); | ||
| } | ||
| }; | ||
|
|
||
| public static void main(String[] args) { | ||
|
|
||
| CountDownLatch latch = new CountDownLatch(2); | ||
|
|
||
| Thread.ofPlatform().start(() -> { | ||
| latch.countDown(); | ||
| try { | ||
| latch.await(); | ||
| } catch (InterruptedException ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| System.out.println("T1 running"); | ||
| for (int i = 0; i < RUNS; i++) { | ||
| payload1(); | ||
| } | ||
| }); | ||
|
|
||
| Thread.ofPlatform().start(() -> { | ||
| latch.countDown(); | ||
| try { | ||
| latch.await(); | ||
| } catch (InterruptedException ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| System.out.println("T2 running"); | ||
| for (int i = 0; i < RUNS; i++) { | ||
| payload2(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public static int payload1() { | ||
| return (int) m(VH_X); | ||
| } | ||
|
|
||
| public static long payload2() { | ||
| return (long) m(VH_Y); | ||
| } | ||
|
|
||
| public static Object m(VarHandle vh) { | ||
| // This is a polymorphic site that sees many VarHandle, but each VH | ||
| // is considered "constant" when inlined into payload1/payload2 | ||
| // payload1/payload2 will throw exceptions if the incorrect VH gets inlined | ||
| return vh.get(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. | ||
| * Copyright (c) 2020, 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
|
|
@@ -69,16 +69,21 @@ public void setup() { | |
|
|
||
| @Benchmark | ||
| public void exact_exactInvocation() { | ||
| exact.set(data, (long) 42); | ||
| var _ = (long) exact.getAndAdd(data, (long) 42); | ||
| } | ||
|
|
||
| @Benchmark | ||
| public void generic_genericInvocation() { | ||
| generic.set(data, 42); | ||
| generic.getAndAdd(data, 42); | ||
| } | ||
|
|
||
| @Benchmark | ||
| public void generic_returnDroppingInvocation() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about "all-generic" case (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change the |
||
| generic.getAndAdd(data, (long) 42); | ||
| } | ||
|
|
||
| @Benchmark | ||
| public void generic_exactInvocation() { | ||
| generic.set(data, (long) 42); | ||
| var _ = (long) generic.getAndAdd(data, (long) 42); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a soft reference needed here? The situation looks similar to
MH.asTypeSoftCache. It can keep some classes referred byvhalive for unnecessarily long.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can use a SoftReference here if we need to achieve constant folding.
Looking at inline_reference_get0, I think we might introduce another field property to trust a reference (potentially in an array) if both that reference and the referent within the reference is non-null. I think that belongs to a separate RFE. What do you think?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it makes sense to limit the caching to safe cases only for now. Otherwise, it would functionally regress due to a possible memory leak.