RFR(XL): 8210685: Bootstrap method consolidation
Karen Kinnear
karen.kinnear at oracle.com
Thu Nov 29 22:03:07 UTC 2018
Note: this is not targeted for JDK 12.
I sent my list of big issues to a smaller list. Repeated here:
Just reviewed Lois’ consolidation of the BSM changes and I have two sets of review comments.
In general, I think the BSM changes include a lot of valuable clean-up which we will want. So many thanks John.
My major concerns are:
1. JVMS implications - is this tracked via a JVMS bug? Who is working on it? Is there a draft updated JVMS?
I don’t see it linked off of JDK-8203252, nor off of JDK-8210685
a) For a BSM that does not require a lookup
b) lazy resolution for BSM arguments
c) rules for repeat resolution of the same entries (see 7c below)
d) JVMS 5.4 Linking - appears to need revising if we are making it possible to perform resolution triggered by an API
A symbolic reference to a dynamically-computed constant is not resolved until either (i) an ldc, ldc_w, or ldc2_w instruction that refers to it is executed, or (ii) a bootstrap method that refers to it as a static argument is invoked.
<>A symbolic reference to a dynamically-computed call site is not resolved until a bootstrap method that refers to it as a static argument is invoked.
2. JVMTI/ class redefinition
a) we worked out an agreement that we could pass cp-indices up to java and cache them and we could arrange
for them to stay unchanged across redefinitions - knowing that the vm could always put a level of indirection in if needed
however - we did not work out a way to handle caching the constantPool itself across redefinitions
It is not clear to me that the new API would want to enforce keeping an older version/pre-redefinition of the constantPool alive,
and continue to reference it. Right now we keep the older versions while they are still executing on the stack. I don’t know
of any way to sync the ConstantPool API with what is on a given stack.
It might make more sense to not cache the vm constantPool in java.
b) JVMTI currently allows removing BootstrapMethod Attribute entries.
JVM_ConstantPool1GetRefAt now returns a bootstrap_methods_attribute_index
but that could be invalidated via redefinition unless we change the JVMTI specification (still an issue for older agents)
c) JVM_ConstantPool1GetRefAt model of returning individual fields rather than a “consistent snapshot"
Lois’ #18 - definitely worth stress testing with JVMTI - to find cases in which parts have changed
3. Lois’ #1 - Graal - that is a release timing challenge.
4. new: public BSCI - I did not see how this is handled in terms of module exports - public to whom?
5. “expression” mode - I could use help with a clearer definition of what this means. Is this the same as Brian’s term “symbolic BSM”?
- is "expression" mode - part 1: requesting symbolic information? Or part 2: resolving values based on that symbolic information? Or both?
- are there tests that explicitly use this? where please?
- is this used in any of Vicente’s code - where please?
6. I would recommending splitting out the JVM_ConstantPool1RefAt - returning of symbolic information vs. returning resolved values to separate APIs.
Then I would like to understand for returning symbolic information:
a) What is the underlying model for who can request symbolic information returned from someone else’s constant pool and what information they
are allowed to have returned?
Why is it ok to create a full-power lookup for expression mode? On whose behalf?
b) What checks are needed here - visibility, checkpackageaccess, runtime access checks, loader constraints?
7. For returning resolved values:
a) What is the underlying model for who can request returning resolved values from someone else’s constant pool and what information they
are allowed to have returned?
b) What checks are needed here - visibility, checkpackageaccess , runtime access checks, loader constraints?
When is it ok to pass a null caller?
c) Today we have JVMS rules for resolving constant pool entries which require repeated same result - success or failure.
My understanding from reading the sources is that the resolved values are not stored in the constant pool - so no guarantee these would
be consistent with local resolution.
Take our favorite example of dynamically increasing access by changing package exports.
thanks,
Karen
p.s. Here are my detailed review comments:
1. use of bootstrap_specifier vs. bsms_attribute has been modified, but doesn’t seem consistent.
2. LinkResolver.hpp
resolve_metadata: since this lazily computes _name/_type perhaps call it resolve_nameandtype
resolve_args -> perhaps resolve_bsm_staticargs
3. LinkResolver.cpp
line 1933: what does DTRT mean? “The resolve_method DTRT with the appendix”
4.jvm.cpp
JVM_ConstantPool1GetRef1 argument “word”
this may go away if we a) split symbolic vs. value and b) make symbolic atomic w.r.t. redefinition
if it stays - perhaps this is unwrapped_sym_index
inconsistent: get_value_at_helper says if get_ref is word#0 - fetch the value - not sure what this means?
get_ref_at_helper says “If word is -1, extract the value of the CP entry itself.
5. ConstantPool::copy_bootstrap_arguments_at_impl
R_SYMREF: returns a typeArrayHandle - appears to return an array of cp indices to symbolic refs
in jvm.cpp JVM_ConstantPool1GetRefAt
appears to return get_ref_at_helper which is a jlString not an interlay
would it make sense to distinguish between R_SYMINDEX and R_SYMREF?
6. jvm.cpp
get_value_at_helper — returns null on exception
old code: JVM_CosntantPoolGetClassAt - appears that if you had is_unresolved_klass_in_error -> you would get
IllegalArgumentException. New code appears to just return null (I didn’t look for an existing test case)
7. jvm.cpp CONSTANTPool1GetDoubleAt: JVM_Wrapper still uses old name
8. jvm.cpp JVM_ConstantPoolGetRefAt
What does the comment mean “Caller must handle symbolic refs other than Utf8?”
9. ciStreams.cpp line 399 “is are” -> “are”
10. BootstrapCallInfo.java
convertResult(BSCI, Object)
// FIXME: If invocationType cannot be resolved, some results are still valid
— what does this mean? when is this used?
11. If the java code needs to know the layout of constant pool entries - does it need to know
the class file version of the constant pool it is working with?
12. BootstrapMethodInvoker
What does “Poorly typed BSM are rare, anyway” mean? What is a Poorly Type BSM and how do they happen?
13. Inconsistency: BAR_SYMREF/BAR_IFPRESENT/BAR_FORCE in java and R_SYMREF/R_IFPRESENT/R_FORCE in vm
I assume BAR - bootstrap argument reference or resolution
14. MethodHandleNatives.java
“If bsm does not begin with Lookup AND it is a CallSite request (legacy from Java 7) - forcibly convert the leading type to Lookup
— what is this example please?
> On Sep 21, 2018, at 3:08 PM, coleen.phillimore at oracle.com wrote:
>
>
> Hi John, I had a first pass look at this code and this is a repeat of the comments I sent earlier.
>
> 271 template(constantPoolOop_name, "constantPoolOop") \
>
> Can you change this name? It implies that the constant pool is an oop or that you've stored it as such. Since you're saving the holder, maybe, poolHolder? This threw me on first look. I wasn't sure what you were saving here.
>
> http://cr.openjdk.java.net/~jrose/jvm/JDK-8210685/src/hotspot/share/interpreter/linkResolver.cpp.frames.html
>
> linkResolver is getting too big! I think BootstrapInfo should be moved to its own new file. Maybe lookup_polymorphic_method could be part of bootstrap info also? Or migrate all of related code in another patch.
>
> 386 tty->print_cr("%s%sBootstrap in %s %s at CP[%d] %s:%s%s BSMS[%d] BSM at CP[%d]%s argc=%d%s",
>
>
> You have tty but you pass in outputStream* st, which is preferable so that we can convert to use UL.
>
> Maybe it makes sense to move the SystemDictionary code to bootstrapInfo.cpp as maybe bootstrapMethods.cpp or something like that.
>
> 926 SystemDictionary::invoke_bootstrap_method(bootstrap_specifier, THREAD);
>
> http://cr.openjdk.java.net/~jrose/jvm/JDK-8210685/src/hotspot/share/oops/cpCache.cpp.frames.html
>
> 693 #ifdef ASSERT
> 694 // invokedynamic and invokehandle have more entries; check if they
> 695 // all point to the same constant pool cache entry.
> 696 for (int entry = 1; entry < ConstantPoolCacheEntry::_indy_resolved_references_entries; entry++) {
> 697 const int cpci_next = invokedynamic_references_map.at(ref + entry);
> 698 assert(cpci == cpci_next, "%d == %d", cpci, cpci_next);
> 699 }
> 700 #endif
>
> I never knew what this did!
>
> http://cr.openjdk.java.net/~jrose/jvm/JDK-8210685/src/hotspot/share/prims/jvm.cpp.frames.html
>
> 2013 // The name "ConstantPool1" means the successor to the original JVM_ConstantPool* API.
>
> These all look like typos in the code! The l and 1 look the same. I can't think of a better name right now, but even 2 is better. or New. Or an underscore.
>
> Or ReflectConstantPool_GetFloatAt. Or how about JVM_CP_GetSize, etc? They should have a different name than the prior constant pool functions since they have different signatures.
>
> All the constant pool specific code in jvm.cpp should be calls into the constantPool.cpp file. That's the file that should know about the details of the constant pool. Especially when we add BlahInError and it's easy to miss some of the case statements in other files.
>
> 2355 constantPoolHandle cp = constantPoolHandle(THREAD, reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));
>
>
> Can you go through and look for constantPoolHandle arguments that are not const references. They should be so that they don't call destructors and copy constructors. I saw a few in the code. Also this line I think will call a copy constructor. It should be:
>
> 2355 constantPoolHandle cp(THREAD, reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));
>
> These handles are not POD anymore (because of redefinition of course).
>
> + enum BootstrapArgumentReferenceMode { R_IFPRESENT = 0, R_FORCE = 1, R_SYMREF = 2 };
>
>
> These are really hard to read, especially in the code context because R is too short. Maybe BR_IFPRESENT. B for Bootstrap. or BOOTREF_IFPRESENT?
>
> http://cr.openjdk.java.net/~jrose/jvm/JDK-8210685/src/java.base/share/classes/jdk/internal/reflect/ConstantPool.java.udiff.html
>
> + int w0 = getWordAt1(index, 0); // refKind
> + int w1 = getWordAt1(index, 1); // member
> + Kind kind = Kind.valueOf(w0, getTagAt(w1) == Tag.INTERFACEMETHODREF);
> + ClassDesc clazz = classDescOf(getUtf8At(w1, 2));
> + return MethodHandleDesc.of(kind, clazz, getUtf8At(w1, 3), getUtf8At(w1, 4));
>
>
> This somewhat crosses the line of knowing how we've stored the MethodHandle in the constant pool. I guess a lot of this code knows the implementation. For this though, why not declare a class like:
>
> class ConstantMethodHandleInfo {
> Object clazz;
> Kind kind;
> String s[2]; // whatever these last 2 args are
> };
>
> And have the JVM fill in an instance of this to return to the Java code. Then it would be more portable to other VMs.
>
> Also here, I think you should keep the native function convention of trailing 0. I don't think naming compatibility is a problem with Java. Maybe the VM code could add the 0s vs. 1, instead of the name I suggested above (?)
>
> + //case FIELDREF:
> + //case METHODREF:
> + //case INTERFACEMETHODREF:
> + //case NAMEANDTYPE:
>
> Why are these commented out here? I thought they were fall through or should be implemented as fall through at first glance. Maybe put them down near the throw IllegalArgumentException so that it's clear that these cases are not supported (yet).
>
> Some parts do look like a good cleanup. I don't really know any of the bootstrap method calling code, though. I'll leave that for the experts.
>
> Thanks,
> Coleen
>
> On 9/13/18 9:03 PM, John Rose wrote:
>> http://cr.openjdk.java.net/~jrose/jvm/JDK-8210685/
>>
>> Java uses of bootstrap methods for indy and condy, and is likely to
>> further modify them. The code in the JVM and JDK needs consolidation.
>>
>> The JDK support code for BSMs is hard to read and maintain. Let's
>> clean it up and simplify it. There should be a single BootstrapInfo
>> struct in the JVM which expresses all BSM-related state, similar to
>> the CallInfo and LinkInfo structs.
>>
>> The handshake from the JVM to JDK code is complicated and
>> ill-documented. Let's simplify that also. If we regularize all BSM
>> paths to use BootstrapCallInfo only, we will lose no generality or
>> performance, and make the code easier to maintain and evolve.
>> It will also be easier to evolve the JVM specification, with regard
>> to bootstrap methods, if new behaviors can be expressed mainly
>> at the JDK level, as the behaviors of the JDK processing BSCI
>> instances.
>>
>> As part of this, let's remove unused JVM paths and data fields. For
>> example, the method_type field associated with the appendix field of
>> an indy or method handle call is unused and just adds complexity and
>> footprint. Also, while we are simplifying the CP support for such
>> call sites, we can rename them to be about BSMs rather than about indy
>> and condy, and we can share more indy and condy code in common.
>>
>> Let's also move bootstrap argument processing from MethodHandleNatives
>> to the pre-existing (low-level, privileged) ConstantPool class. That
>> also unlocks the capability for BSMs to gain direct access to CP
>> structures.
>>
>> Remove ConstantGroup; it was a bad idea. Instead, merge argument
>> access into BootstrapCallInfo, back-ending to ConstantPool. Of
>> course, the JVM should continue to pre-load argument lists.
>>
>> Adjust BSM argument access: remove copyArguments in favor of reusing
>> the List.toArray protocol. Add an argumentRef API for direct access
>> to BSM arguments in symbolic form. Also, add invocationTypeRef to
>> support pre-resolved types (method and field), so that non-resolvable
>> types can be processed.
>>
>> The Amber work has uncovered the need for new BSM modes which can omit
>> metadata (lookup, name, type). Implement this as an open-ended
>> "expression mode" to go with the legacy "push mode" and universal
>> "BootstrapCallInfo" modes; this gives it some room to grow in the
>> future.
>>
>> About half of this work is in the JVM, half in the lower levels of the JDK.
>> New Java APIs can be rolled out over time, but need not be public at first.
>>
>> User-visible "expression mode" BSMs will be a feature of this work.
>> A separate CCC may be needed to review these user-visible changes,
>> or it may be "rolled into" the JVM Constants API JDK-8210031, which
>> is under separate review by amber-dev and compiler-dev:
>>
>> http://mail.openjdk.java.net/pipermail/compiler-dev/2018-September/012388.html
>>
>> The present review is for JVM-related changes, including to the JDK
>> code that manages bootstrap method invocation, which is intimately
>> coupled to the JVM.
>>
>> — John
>
More information about the hotspot-runtime-dev
mailing list