Skip to content
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

8312235: [JVMCI] ConstantPool should not force eager resolution #14927

Closed
wants to merge 7 commits into from
35 changes: 23 additions & 12 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -697,15 +697,24 @@ C2V_VMENTRY_NULL(jobject, getUncachedStringInPool, (JNIEnv* env, jobject, ARGUME
return JVMCIENV->get_jobject(JVMCIENV->get_object_constant(obj));
C2V_END

C2V_VMENTRY_NULL(jobject, resolvePossiblyCachedConstantInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index))
C2V_VMENTRY_NULL(jobject, lookupConstantInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint cp_index, bool resolve))
constantPoolHandle cp(THREAD, UNPACK_PAIR(ConstantPool, cp));
oop obj = cp->resolve_possibly_cached_constant_at(index, CHECK_NULL);
constantTag tag = cp->tag_at(index);
oop obj;
if (!resolve) {
bool found_it;
obj = cp->find_cached_constant_at(cp_index, found_it, CHECK_NULL);
if (!found_it) {
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be some sort of exception or error checking here or in the caller? If the constant can't be found it either isn't resolved or the index is invalid, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error checking is already done by the call to getTagAt in HotSpotConstantPool.lookupConstant(int cpi, boolean resolve).
I'll add more javadoc to clarify when a null is returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the javadoc of lookupConstantInPool already states:

     * The behavior of this method is undefined if {@code cpi} does not denote one of the following
     * entry types: {@code JVM_CONSTANT_Dynamic}, {@code JVM_CONSTANT_String},
     * {@code JVM_CONSTANT_MethodHandle}, {@code JVM_CONSTANT_MethodHandleInError},
     * {@code JVM_CONSTANT_MethodType} and {@code JVM_CONSTANT_MethodTypeInError}.

}
} else {
obj = cp->resolve_possibly_cached_constant_at(cp_index, CHECK_NULL);
}
constantTag tag = cp->tag_at(cp_index);
if (tag.is_dynamic_constant()) {
if (obj == nullptr) {
return JVMCIENV->get_jobject(JVMCIENV->get_JavaConstant_NULL_POINTER());
}
BasicType bt = Signature::basic_type(cp->uncached_signature_ref_at(index));
BasicType bt = Signature::basic_type(cp->uncached_signature_ref_at(cp_index));
if (!is_reference_type(bt)) {
if (!is_java_primitive(bt)) {
return JVMCIENV->get_jobject(JVMCIENV->get_JavaConstant_ILLEGAL());
Expand Down Expand Up @@ -1578,16 +1587,18 @@ C2V_VMENTRY_NULL(jobject, iterateFrames, (JNIEnv* env, jobject compilerToVM, job
return nullptr;
C2V_END

C2V_VMENTRY_0(int, resolveInvokeDynamicInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index))
if (!ConstantPool::is_invokedynamic_index(index)) {
JVMCI_THROW_MSG_0(IllegalStateException, err_msg("not an invokedynamic index %d", index));
C2V_VMENTRY_0(int, decodeIndyIndexToCPIndex, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint encoded_indy_index, jboolean resolve))
if (!ConstantPool::is_invokedynamic_index(encoded_indy_index)) {
JVMCI_THROW_MSG_0(IllegalStateException, err_msg("not an encoded indy index %d", encoded_indy_index));
}

constantPoolHandle cp(THREAD, UNPACK_PAIR(ConstantPool, cp));
CallInfo callInfo;
LinkResolver::resolve_invoke(callInfo, Handle(), cp, index, Bytecodes::_invokedynamic, CHECK_0);
int indy_index = cp->decode_invokedynamic_index(index);
cp->cache()->set_dynamic_call(callInfo, indy_index);
int indy_index = cp->decode_invokedynamic_index(encoded_indy_index);
if (resolve) {
LinkResolver::resolve_invoke(callInfo, Handle(), cp, encoded_indy_index, Bytecodes::_invokedynamic, CHECK_0);
cp->cache()->set_dynamic_call(callInfo, indy_index);
}
return cp->resolved_indy_entry_at(indy_index)->constant_pool_index();
C2V_END

Expand Down Expand Up @@ -3108,13 +3119,13 @@ JNINativeMethod CompilerToVM::methods[] = {
{CC "lookupKlassInPool", CC "(" HS_CONSTANT_POOL2 "I)Ljava/lang/Object;", FN_PTR(lookupKlassInPool)},
{CC "lookupAppendixInPool", CC "(" HS_CONSTANT_POOL2 "I)" OBJECTCONSTANT, FN_PTR(lookupAppendixInPool)},
{CC "lookupMethodInPool", CC "(" HS_CONSTANT_POOL2 "IB" HS_METHOD2 ")" HS_METHOD, FN_PTR(lookupMethodInPool)},
{CC "lookupConstantInPool", CC "(" HS_CONSTANT_POOL2 "IZ)" JAVACONSTANT, FN_PTR(lookupConstantInPool)},
{CC "constantPoolRemapInstructionOperandFromCache", CC "(" HS_CONSTANT_POOL2 "I)I", FN_PTR(constantPoolRemapInstructionOperandFromCache)},
{CC "resolveBootstrapMethod", CC "(" HS_CONSTANT_POOL2 "I)[" OBJECT, FN_PTR(resolveBootstrapMethod)},
{CC "getUncachedStringInPool", CC "(" HS_CONSTANT_POOL2 "I)" JAVACONSTANT, FN_PTR(getUncachedStringInPool)},
{CC "resolvePossiblyCachedConstantInPool", CC "(" HS_CONSTANT_POOL2 "I)" JAVACONSTANT, FN_PTR(resolvePossiblyCachedConstantInPool)},
{CC "resolveTypeInPool", CC "(" HS_CONSTANT_POOL2 "I)" HS_KLASS, FN_PTR(resolveTypeInPool)},
{CC "resolveFieldInPool", CC "(" HS_CONSTANT_POOL2 "I" HS_METHOD2 "B[I)" HS_KLASS, FN_PTR(resolveFieldInPool)},
{CC "resolveInvokeDynamicInPool", CC "(" HS_CONSTANT_POOL2 "I)I", FN_PTR(resolveInvokeDynamicInPool)},
{CC "decodeIndyIndexToCPIndex", CC "(" HS_CONSTANT_POOL2 "IZ)I", FN_PTR(decodeIndyIndexToCPIndex)},
{CC "resolveInvokeHandleInPool", CC "(" HS_CONSTANT_POOL2 "I)V", FN_PTR(resolveInvokeHandleInPool)},
{CC "isResolvedInvokeHandleInPool", CC "(" HS_CONSTANT_POOL2 "I)I", FN_PTR(isResolvedInvokeHandleInPool)},
{CC "resolveMethod", CC "(" HS_KLASS2 HS_METHOD2 HS_KLASS2 ")" HS_METHOD, FN_PTR(resolveMethod)},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,19 +271,22 @@ JavaConstant getUncachedStringInPool(HotSpotConstantPool constantPool, int cpi)
private native JavaConstant getUncachedStringInPool(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi);

/**
* Resolves the entry at index {@code cpi} in {@code constantPool} to an object, looking in the
* Gets the entry at index {@code cpi} in {@code constantPool}, looking in the
* constant pool cache first.
*
* The behavior of this method is undefined if {@code cpi} does not denote one of the following
* entry types: {@code JVM_CONSTANT_Dynamic}, {@code JVM_CONSTANT_String},
* {@code JVM_CONSTANT_MethodHandle}, {@code JVM_CONSTANT_MethodHandleInError},
* {@code JVM_CONSTANT_MethodType} and {@code JVM_CONSTANT_MethodTypeInError}.
*
* @param resolve specifies if a resolved entry is expected. If {@code false},
* {@code null} is returned for an unresolved entry.
*/
JavaConstant resolvePossiblyCachedConstantInPool(HotSpotConstantPool constantPool, int cpi) {
return resolvePossiblyCachedConstantInPool(constantPool, constantPool.getConstantPoolPointer(), cpi);
JavaConstant lookupConstantInPool(HotSpotConstantPool constantPool, int cpi, boolean resolve) {
return lookupConstantInPool(constantPool, constantPool.getConstantPoolPointer(), cpi, resolve);
}

private native JavaConstant resolvePossiblyCachedConstantInPool(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi);
private native JavaConstant lookupConstantInPool(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi, boolean resolve);

/**
* Gets the {@code JVM_CONSTANT_NameAndType} index referenced by the {@code rawIndex}.
Expand Down Expand Up @@ -387,17 +390,18 @@ private native HotSpotResolvedJavaMethodImpl lookupMethodInPool(HotSpotConstantP
long callerMethodPointer);

/**
* Ensures that the type referenced by the specified {@code JVM_CONSTANT_InvokeDynamic} entry at
* index {@code cpi} in {@code constantPool} is loaded and initialized.
* Converts the encoded indy index operand of an invokedynamic instruction
* to an index directly into {@code constantPool}.
*
* @throws IllegalArgumentException if {@code cpi} is not an invokedynamic index
* @return the invokedynamic index
* @param resolve if {@true}, then resolve the entry (which may call a bootstrap method)
* @throws IllegalArgumentException if {@code encoded_indy_index} is not an encoded indy index
* @return {@code JVM_CONSTANT_InvokeDynamic} constant pool entry index for the invokedynamic
*/
int resolveInvokeDynamicInPool(HotSpotConstantPool constantPool, int cpi) {
return resolveInvokeDynamicInPool(constantPool, constantPool.getConstantPoolPointer(), cpi);
int decodeIndyIndexToCPIndex(HotSpotConstantPool constantPool, int encoded_indy_index, boolean resolve) {
return decodeIndyIndexToCPIndex(constantPool, constantPool.getConstantPoolPointer(), encoded_indy_index, resolve);
}

private native int resolveInvokeDynamicInPool(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi);
private native int decodeIndyIndexToCPIndex(HotSpotConstantPool constantPool, long constantPoolPointer, int encoded_indy_index, boolean resolve);

/**
* Resolves the details for invoking the bootstrap method associated with the
Expand Down Expand Up @@ -440,7 +444,7 @@ void resolveInvokeHandleInPool(HotSpotConstantPool constantPool, int cpi) {

/**
* If {@code cpi} denotes an entry representing a resolved dynamic adapter (see
* {@link #resolveInvokeDynamicInPool} and {@link #resolveInvokeHandleInPool}), return the
* {@link #decodeIndyIndexToCPIndex} and {@link #resolveInvokeHandleInPool}), return the
* opcode of the instruction for which the resolution was performed ({@code invokedynamic} or
* {@code invokevirtual}), or {@code -1} otherwise.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,11 @@ JavaConstant getStaticFieldConstantValue(int cpi) {

@Override
public Object lookupConstant(int cpi) {
return lookupConstant(cpi, true);
}

@Override
public Object lookupConstant(int cpi, boolean resolve) {
final JvmConstant tag = getTagAt(cpi);
switch (tag.name) {
case "Integer":
Expand All @@ -658,17 +663,18 @@ public Object lookupConstant(int cpi) {
* "pseudo strings" (arbitrary live objects) patched into a String entry. Such
* entries do not have a symbol in the constant pool slot.
*/
return compilerToVM().resolvePossiblyCachedConstantInPool(this, cpi);
return compilerToVM().lookupConstantInPool(this, cpi, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true because it's always safe to do so?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know what these "pseudo strings" are. In any case, I don't think resolving them involves calling any Java code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. It's not currently causing any problems and we can investigate further if we start restricting when we can_call_java and problems show up here.

case "MethodHandle":
case "MethodHandleInError":
case "MethodType":
case "MethodTypeInError":
case "Dynamic":
case "DynamicInError":
return compilerToVM().resolvePossiblyCachedConstantInPool(this, cpi);
return compilerToVM().lookupConstantInPool(this, cpi, resolve);
default:
throw new JVMCIError("Unknown constant pool tag %s", tag);
}

}

@Override
Expand Down Expand Up @@ -822,7 +828,7 @@ public int rawIndexToConstantPoolIndex(int rawIndex, int opcode) {
if (opcode != Bytecodes.INVOKEDYNAMIC) {
throw new IllegalArgumentException("expected INVOKEDYNAMIC at " + rawIndex + ", got " + opcode);
}
return compilerToVM().resolveInvokeDynamicInPool(this, rawIndex);
return compilerToVM().decodeIndyIndexToCPIndex(this, rawIndex, false);
}
if (opcode == Bytecodes.INVOKEDYNAMIC) {
throw new IllegalArgumentException("unexpected INVOKEDYNAMIC at " + rawIndex);
Expand Down Expand Up @@ -856,7 +862,7 @@ public void loadReferencedType(int cpi, int opcode, boolean initialize) {
if (!isInvokedynamicIndex(cpi)) {
throw new IllegalArgumentException("must use invokedynamic index but got " + cpi);
}
index = compilerToVM().resolveInvokeDynamicInPool(this, cpi);
index = compilerToVM().decodeIndyIndexToCPIndex(this, cpi, true);
break;
}
case Bytecodes.GETSTATIC:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,20 @@ default BootstrapMethodInvocation lookupBootstrapMethodInvocation(int cpi, int o
*/
Object lookupConstant(int cpi);

/**
* Looks up a constant at the specified index.
*
* If {@code resolve == false} and the denoted constant is of type
* {@code JVM_CONSTANT_Dynamic}, {@code JVM_CONSTANT_MethodHandle} or
* {@code JVM_CONSTANT_MethodType} and it's not yet resolved then
* {@code null} is returned.
*
* @param cpi the constant pool index
* @return the {@code Constant} or {@code JavaType} instance representing the constant pool
* entry
*/
Object lookupConstant(int cpi, boolean resolve);

/**
* Looks up the appendix at the specified index.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ public static HotSpotResolvedObjectType lookupTypeHelper(String name,
}
}

public static Object resolvePossiblyCachedConstantInPool(ConstantPool constantPool, int cpi) {
DirectHotSpotObjectConstantImpl obj = (DirectHotSpotObjectConstantImpl) CTVM.resolvePossiblyCachedConstantInPool((HotSpotConstantPool) constantPool, cpi);
public static Object lookupConstantInPool(ConstantPool constantPool, int cpi, boolean resolve) {
DirectHotSpotObjectConstantImpl obj = (DirectHotSpotObjectConstantImpl) CTVM.lookupConstantInPool((HotSpotConstantPool) constantPool, cpi, resolve);
return obj.object;
}

Expand Down Expand Up @@ -132,11 +132,6 @@ public static HotSpotResolvedJavaMethod lookupMethodInPool(
return CTVM.lookupMethodInPool((HotSpotConstantPool) constantPool, cpi, opcode, null);
}

public static void resolveInvokeDynamicInPool(
ConstantPool constantPool, int cpi) {
CTVM.resolveInvokeDynamicInPool((HotSpotConstantPool) constantPool, cpi);
}

public static void resolveInvokeHandleInPool(
ConstantPool constantPool, int cpi) {
CTVM.resolveInvokeHandleInPool((HotSpotConstantPool) constantPool, cpi);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 2023, 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
Expand Down Expand Up @@ -41,7 +41,7 @@
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions
* -XX:+WhiteBoxAPI -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI
* -XX:-UseJVMCICompiler
* compiler.jvmci.compilerToVM.ResolvePossiblyCachedConstantInPoolTest
* compiler.jvmci.compilerToVM.LookupConstantInPoolTest
*/

package compiler.jvmci.compilerToVM;
Expand All @@ -64,15 +64,15 @@
import static compiler.jvmci.compilerToVM.ConstantPoolTestCase.ConstantTypes.CONSTANT_STRING;

/**
* Test for {@code jdk.vm.ci.hotspot.CompilerToVM.resolvePossiblyCachedConstantInPool} method
* Test for {@code jdk.vm.ci.hotspot.CompilerToVM.lookupConstantInPool} method
*/
public class ResolvePossiblyCachedConstantInPoolTest {
public class LookupConstantInPoolTest {

public static void main(String[] args) throws Exception {
Map<ConstantTypes, Validator> typeTests = new HashMap<>();
typeTests.put(CONSTANT_STRING, ResolvePossiblyCachedConstantInPoolTest::validateString);
typeTests.put(CONSTANT_METHODHANDLE, ResolvePossiblyCachedConstantInPoolTest::validateMethodHandle);
typeTests.put(CONSTANT_METHODTYPE, ResolvePossiblyCachedConstantInPoolTest::validateMethodType);
typeTests.put(CONSTANT_STRING, LookupConstantInPoolTest::validateString);
typeTests.put(CONSTANT_METHODHANDLE, LookupConstantInPoolTest::validateMethodHandle);
typeTests.put(CONSTANT_METHODTYPE, LookupConstantInPoolTest::validateMethodType);
ConstantPoolTestCase testCase = new ConstantPoolTestCase(typeTests);
testCase.test();
// The next "Class.forName" and repeating "testCase.test()"
Expand Down Expand Up @@ -103,7 +103,7 @@ private static void validateString(ConstantPool constantPoolCTVM,
index = cpci;
cached = "cached ";
}
Object constantInPool = CompilerToVMHelper.resolvePossiblyCachedConstantInPool(constantPoolCTVM, index);
Object constantInPool = CompilerToVMHelper.lookupConstantInPool(constantPoolCTVM, index, true);
String stringToVerify = (String) constantInPool;
String stringToRefer = entry.name;
if (stringToRefer.equals("") && cpci != ConstantPoolTestsHelper.NO_CP_CACHE_PRESENT) {
Expand All @@ -114,14 +114,14 @@ private static void validateString(ConstantPool constantPoolCTVM,
}

private static final String NOT_NULL_MSG
= "Object returned by resolvePossiblyCachedConstantInPool method should not be null";
= "Object returned by lookupConstantInPool method should not be null";


private static void validateMethodHandle(ConstantPool constantPoolCTVM,
ConstantTypes cpType,
DummyClasses dummyClass,
int index) {
Object constantInPool = CompilerToVMHelper.resolvePossiblyCachedConstantInPool(constantPoolCTVM, index);
Object constantInPool = CompilerToVMHelper.lookupConstantInPool(constantPoolCTVM, index, true);
String msg = String.format("%s for index %d", NOT_NULL_MSG, index);
Asserts.assertNotNull(constantInPool, msg);
if (!(constantInPool instanceof MethodHandle)) {
Expand All @@ -138,7 +138,7 @@ private static void validateMethodType(ConstantPool constantPoolCTVM,
ConstantTypes cpType,
DummyClasses dummyClass,
int index) {
Object constantInPool = CompilerToVMHelper.resolvePossiblyCachedConstantInPool(constantPoolCTVM, index);
Object constantInPool = CompilerToVMHelper.lookupConstantInPool(constantPoolCTVM, index, true);
String msg = String.format("%s for index %d", NOT_NULL_MSG, index);
Asserts.assertNotNull(constantInPool, msg);
Class mtToVerify = constantInPool.getClass();
Expand Down
Loading