RFR(XL): 8210685: Bootstrap method consolidation

[Claes Redestad]

Hi,

not a full review, only started looking at low-level JDK code and taking 
this large patch for a spin..

MethodHandleNatives calls with Type null into a TypeView constructor 
that will unconditionally throw NPE.
I suspect you intended to call the currently unused 
(ConstantDesc,Lookup) constructor?

diff -r a0f48062d3a6 
src/java.base/share/classes/java/lang/invoke/MethodHandleNatives.java
--- 
a/src/java.base/share/classes/java/lang/invoke/MethodHandleNatives.java 
Fri Sep 14 14:21:28 2018 +0200
+++ 
b/src/java.base/share/classes/java/lang/invoke/MethodHandleNatives.java 
Fri Sep 14 14:24:33 2018 +0200
@@ -272,6 +272,8 @@
              bsm = BootstrapMethodInvoker.forceLookupParameter(bsm);
assert(!BootstrapMethodInvoker.isExpressionBSM(bsm));  // properly wrapped
          }
+        // Separately, make a full-power lookup into the CP.
+        Lookup lookup = IMPL_LOOKUP.in(pool.getHolder());
          // Process the type:  { field, method } x { resolved, string }
          TypeView<Class<?>>   ftype = null;
          TypeView<MethodType> mtype = null;
@@ -283,9 +285,9 @@
              // JVM was not able to resolve the type.  Keep the ball 
rolling.
              String desc = (String) type;
              if (desc.startsWith("("))
-                mtype = new TypeView<>(null, 
MethodTypeDesc.ofDescriptor(desc));
+                mtype = new 
TypeView<>(MethodTypeDesc.ofDescriptor(desc), lookup);
              else
-                ftype = new TypeView<>(null, 
ClassDesc.ofDescriptor(desc.replace('/', '.')));
+                ftype = new 
TypeView<>(ClassDesc.ofDescriptor(desc.replace('/', '.')), lookup);
          }
          // If VM is pushing up argument values, use them. Otherwise 
make a fresh buffer.
          if (argValues == null) {
@@ -304,8 +306,6 @@
              bsci = new VM_BSCI<>(pool, bssIndex, indyIndex, bsm, name, 
mtype, argValues);
          // If VM is passing up argument indexes, accept that also.
          if (argIndexes != null)  bsci.setArgIndexes(argIndexes);
-        // Separately, make a full-power lookup into the CP.
-        Lookup lookup = IMPL_LOOKUP.in(pool.getHolder());
          if (!TRACE_METHOD_LINKAGE) {
              Object res = BootstrapMethodInvoker.invoke(lookup, bsci);
              if (appendixResult == null) {

I guess this case needs a test.. :)

Allocation of TypeViews, VM_BSCI, creating a List of arguments from 
VM_BSCI/BootstrapCallInfo to then
subList and finally create an argument array in 
BootstrapMethodInvoker::invokeCommon... these particular
tiny overheads *do* add up (and show up in startup profiles), and I can 
see a measurable regression on
tests that stress-tests bootstraps with arguments, such as indified 
string concats. Anything of this
that could be simplified or improved to maintain comparable performance 
characteristics?

Thanks!

/Claes

On 2018-09-14 03:03, 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