Skip to content

8370251: C2: Inlining checks for method handle intrinsics are too strict #27908

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+126 −43
Open

8370251: C2: Inlining checks for method handle intrinsics are too strict #27908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/hotspot/share/opto/doCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
(orig_callee->intrinsic_id() == vmIntrinsics::_linkToVirtual) ||
(orig_callee->intrinsic_id() == vmIntrinsics::_linkToInterface);

const bool check_access = !orig_callee->is_method_handle_intrinsic(); // method handle intrinsics don't perform access checks

// Dtrace currently doesn't work unless all calls are vanilla
if (env()->dtrace_method_probes()) {
allow_inline = false;
Expand Down Expand Up @@ -239,7 +241,8 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
// this invoke because it may lead to bimorphic inlining which
// a speculative type should help us avoid.
receiver_method = callee->resolve_invoke(jvms->method()->holder(),
speculative_receiver_type);
speculative_receiver_type,
check_access);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why only here you pass check_access and expect it is true in all other places?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question, should we add an assert for check_access before the resolve_invoke in Compile::optimize_inlining?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why only here you pass check_access and expect it is true in all other places?

@vnkozlov That's the only case which was overlooked in JDK-8062280. All other cases aren't exercised for MH intrinsic methods and the asserts are there to verify that. If they start to fail, it'll signal that there may be a missing optimization opportunity.

should we add an assert for check_access before the resolve_invoke in Compile::optimize_inlining?

@liach good question, it makes sense to separately take a closer look at this particular case. My first impression is check_access should be passed into resolve_invoke rather than asserting check_access == true before resolve_invoke.

if (receiver_method == nullptr) {
speculative_receiver_type = nullptr;
} else {
Expand All @@ -256,8 +259,9 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
(morphism == 2 && UseBimorphicInlining))) {
// receiver_method = profile.method();
// Profiles do not suggest methods now. Look it up in the major receiver.
assert(check_access, "required");
receiver_method = callee->resolve_invoke(jvms->method()->holder(),
profile.receiver(0));
profile.receiver(0));
}
if (receiver_method != nullptr) {
// The single majority receiver sufficiently outweighs the minority.
Expand All @@ -268,8 +272,9 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
CallGenerator* next_hit_cg = nullptr;
ciMethod* next_receiver_method = nullptr;
if (morphism == 2 && UseBimorphicInlining) {
assert(check_access, "required");
next_receiver_method = callee->resolve_invoke(jvms->method()->holder(),
profile.receiver(1));
profile.receiver(1));
if (next_receiver_method != nullptr) {
next_hit_cg = this->call_generator(next_receiver_method,
vtable_index, !call_does_dispatch, jvms,
Expand Down Expand Up @@ -342,6 +347,7 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
if (singleton != nullptr) {
assert(singleton != declared_interface, "not a unique implementor");

assert(check_access, "required");
ciMethod* cha_monomorphic_target =
callee->find_monomorphic_target(caller->holder(), declared_interface, singleton);

Expand Down
157 changes: 117 additions & 40 deletions test/hotspot/jtreg/compiler/jsr292/MHInlineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ public static void main(String[] args) throws Exception {
"-XX:+IgnoreUnrecognizedVMOptions", "-showversion",
"-XX:-TieredCompilation", "-Xbatch",
"-XX:+PrintCompilation", "-XX:+UnlockDiagnosticVMOptions", "-XX:+PrintInlining",
"-XX:CompileCommand=dontinline,compiler.jsr292.MHInlineTest::test*",
"-XX:CompileCommand=quiet",
"-XX:CompileCommand=compileonly,compiler.jsr292.MHInlineTest::test*",
Launcher.class.getName());

OutputAnalyzer analyzer = new OutputAnalyzer(pb.start());
Expand All @@ -60,13 +61,17 @@ public static void main(String[] args) throws Exception {

// The test is applicable only to C2 (present in Server VM).
if (analyzer.getStderr().contains("Server VM")) {
analyzer.shouldContain("compiler.jsr292.MHInlineTest$A::package_final_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$A::private_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$A::package_static_x (3 bytes) inline (hot)");

analyzer.shouldContain("compiler.jsr292.MHInlineTest$B::public_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$B::protected_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$B::package_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$A::package_final_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$B::private_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$B::private_static_x (3 bytes) inline (hot)");
analyzer.shouldContain("compiler.jsr292.MHInlineTest$A::package_static_x (3 bytes) inline (hot)");

analyzer.shouldNotContain("failed to inline");
} else {
throw new SkippedException("The test is applicable only to C2 (present in Server VM)");
}
Expand All @@ -75,30 +80,32 @@ public static void main(String[] args) throws Exception {
static class A {
public static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();

public Class<?> public_x() { return A.class; }
protected Class<?> protected_x() { return A.class; }
Class<?> package_x() { return A.class; }
final Class<?> package_final_x() { return A.class; }
public Class<?> public_x() { return A.class; }
protected Class<?> protected_x() { return A.class; }
/*package*/ Class<?> package_x() { return A.class; }
/*package*/ final Class<?> package_final_x() { return A.class; }
private Class<?> private_x() { return A.class; }

static Class<?> package_static_x() { return A.class; }
/*package*/ static Class<?> package_static_x() { return A.class; }
}

static class B extends A {
public static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();

@Override public Class<?> public_x() { return B.class; }
@Override protected Class<?> protected_x() { return B.class; }
@Override Class<?> package_x() { return B.class; }
@Override public Class<?> public_x() { return B.class; }
@Override protected Class<?> protected_x() { return B.class; }
@Override /*package*/ Class<?> package_x() { return B.class; }

private Class<?> private_x() { return B.class; }
static Class<?> private_static_x() { return B.class; }
private Class<?> private_x() { return B.class; }
private static Class<?> private_static_x() { return B.class; }
}

static final MethodHandle A_PUBLIC_X;
static final MethodHandle A_PROTECTED_X;
static final MethodHandle A_PACKAGE_X;
static final MethodHandle A_PACKAGE_STATIC_X;
static final MethodHandle A_PACKAGE_FINAL_X;
static final MethodHandle A_PRIVATE_X;

static final MethodHandle B_PRIVATE_X;
static final MethodHandle B_PRIVATE_STATIC_X;
Expand All @@ -117,6 +124,8 @@ static class B extends A {
A.class, "package_final_x", MethodType.methodType(Class.class));
A_PACKAGE_STATIC_X = LOOKUP.findStatic(
A.class, "package_static_x", MethodType.methodType(Class.class));
A_PRIVATE_X = LOOKUP.findVirtual(
A.class, "private_x", MethodType.methodType(Class.class));

B_PRIVATE_X = B.LOOKUP.findVirtual(
B.class, "private_x", MethodType.methodType(Class.class));
Expand All @@ -129,7 +138,18 @@ static class B extends A {

static final A a = new B();

private static void testPublicMH() {
/* ============== public Class<?> public_x() ============== */

private static void testPublicMH(A recv) {
try {
Class<?> r = (Class<?>)A_PUBLIC_X.invokeExact(recv);
assertEquals(r, B.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPublicMHConst() {
try {
Class<?> r = (Class<?>)A_PUBLIC_X.invokeExact(a);
assertEquals(r, B.class);
Expand All @@ -138,7 +158,18 @@ private static void testPublicMH() {
}
}

private static void testProtectedMH() {
/* ============== protected Class<?> protected_x() ============== */

private static void testProtectedMH(A recv) {
try {
Class<?> r = (Class<?>)A_PROTECTED_X.invokeExact(recv);
assertEquals(r, B.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testProtectedMHConst() {
try {
Class<?> r = (Class<?>)A_PROTECTED_X.invokeExact(a);
assertEquals(r, B.class);
Expand All @@ -147,7 +178,18 @@ private static void testProtectedMH() {
}
}

private static void testPackageMH() {
/* ============== Class<?> package_x() ============== */

private static void testPackageMH(A recv) {
try {
Class<?> r = (Class<?>)A_PACKAGE_X.invokeExact(recv);
assertEquals(r, B.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPackageMHConst() {
try {
Class<?> r = (Class<?>)A_PACKAGE_X.invokeExact(a);
assertEquals(r, B.class);
Expand All @@ -156,7 +198,9 @@ private static void testPackageMH() {
}
}

private static void testPackageFinalMH() {
/* ============== final Class<?> package_final_x() ============== */

private static void testPackageFinalMH(A recv) {
try {
Class<?> r = (Class<?>)A_PACKAGE_FINAL_X.invokeExact(a);
assertEquals(r, A.class);
Expand All @@ -165,16 +209,45 @@ private static void testPackageFinalMH() {
}
}

private static void testPackageStaticMH() {
private static void testPackageFinalMHConst() {
try {
Class<?> r = (Class<?>)A_PACKAGE_STATIC_X.invokeExact();
Class<?> r = (Class<?>)A_PACKAGE_FINAL_X.invokeExact(a);
assertEquals(r, A.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPrivateMH() {
/* ============== private Class<?> private_x() ============== */

private static void testPrivateA_MH(A recv) {
try {
Class<?> r = (Class<?>)A_PRIVATE_X.invokeExact(recv);
assertEquals(r, A.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPrivateA_MHConst() {
try {
Class<?> r = (Class<?>)A_PRIVATE_X.invokeExact(a);
assertEquals(r, A.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPrivateB_MH(B recv) {
try {
Class<?> r = (Class<?>)B_PRIVATE_X.invokeExact(recv);
assertEquals(r, B.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}

private static void testPrivateB_MHConst() {
try {
Class<?> r = (Class<?>)B_PRIVATE_X.invokeExact((B)a);
assertEquals(r, B.class);
Expand All @@ -183,7 +256,19 @@ private static void testPrivateMH() {
}
}

private static void testPrivateStaticMH() {
/* ============== static ============== */

private static void testPackageStaticMHConst() {
try {
Class<?> r = (Class<?>)A_PACKAGE_STATIC_X.invokeExact();
assertEquals(r, A.class);
} catch (Throwable throwable) {
throw new Error(throwable);
}
}


private static void testPrivateStaticMHConst() {
try {
Class<?> r = (Class<?>)B_PRIVATE_STATIC_X.invokeExact();
assertEquals(r, B.class);
Expand All @@ -192,28 +277,20 @@ private static void testPrivateStaticMH() {
}
}

/* ====================================================================== */

static class Launcher {
public static void main(String[] args) throws Exception {
for (int i = 0; i < 20_000; i++) {
testPublicMH();
}
for (int i = 0; i < 20_000; i++) {
testProtectedMH();
}
for (int i = 0; i < 20_000; i++) {
testPackageMH();
}
for (int i = 0; i < 20_000; i++) {
testPackageFinalMH();
}
for (int i = 0; i < 20_000; i++) {
testPackageStaticMH();
}
for (int i = 0; i < 20_000; i++) {
testPrivateMH();
}
for (int i = 0; i < 20_000; i++) {
testPrivateStaticMH();
testPublicMH(a); testPublicMHConst();
testProtectedMH(a); testProtectedMHConst();
testPackageMH(a); testPackageMHConst();
testPackageFinalMH(a); testPackageFinalMHConst();
testPrivateA_MH(a); testPrivateA_MHConst();
testPrivateB_MH((B)a); testPrivateB_MHConst();

testPackageStaticMHConst();
testPrivateStaticMHConst();
}
}
}
Expand Down