RFR(XL): 8210685: Bootstrap method consolidation

[coleen.phillimore at oracle.com]

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