[9] RFR(L) 8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames

[John Rose]

On Oct 17, 2014, at 11:16 AM, David Chase <david.r.chase at oracle.com> wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8013267
> 
> webrevs:
> http://cr.openjdk.java.net/~drchase/8013267/jdk.03/
> http://cr.openjdk.java.net/~drchase/8013267/hotspot.03/
> 
> related bug: https://bugs.openjdk.java.net/browse/JDK-8042235

This is a big 'un.  Now that we can modify hotspot and jdk together more easily, it's more feasible to work on.  I'm glad to see you taking this on.

A few comments for background:

This is a case where more logic should go at the JDK layer, where we can work on it and understand it more readily.

The JVM layer should supply only a few things for this:
 - A mapping from symbolic references to resolved references
 - A mapping from reflective stuff (jlr.Method) to resolved references
 - A bulk query from a class to its resolved references

The common thread is that only the JVM knows how to inject metadata into the MemberName type.  Other logic, such as caching or interning or registration, is best handled by Java code.

To complicate things, the JVM also needs to (very infrequently) visit the set of all MemberName's in the world.  (Or else we need a turtle under the MN turtle, which we are not doing, thankfully.)  That's why there needs to be a coupling where the JVM can "see" the Class.ClassData fields.

> 1) on the hotspot side, code to adjust the membername table needed to check
>    that the data for the membername table was actually allocated (one goal of this
>    rfe was to avoid allocating that table) else it would null deref and crash.

Nice catch.  C code doesn't do NPEs very well.

> 2) on the jdk side, fencepost error in the membername table insertion; the first
>    entry could be duplicated (  “if index > 0” wrong, should be “if index >= 0”).

Good.

> 3) on the jdk side, several of the constructors for MemberNames needed to be
>    replaced with factories that would consult the MemberNameTable and avoid
>    creating duplicates; again, that was the entire point of this fix.  *NOT ALL
>    CONSTRUCTORS WERE MODIFIED LIKE THIS”, and there was one curious
>    piece of code where it seemed that this might not be what was desired, but it
>    was unclear how this would behave in the face of method redefinition.

Maybe this was a place where the patch was incomplete.

Since the MemberName type is *private* to the java.lang.invoke package, public constructors are tolerable.  (Some or all of them could be made non-public or private, if that makes the code easier to reason about.)

But among the non-private constructors there is a deep difference between constructors which create an unresolved symbolic reference and constructors which somehow create a MN which is resolved from the start.  The former are needed to compose a query to the resolve (and possibly intern) functions.  The latter are special cases needed by the user-visible unreflectFoo methods.

> From 
> 
> http://cr.openjdk.java.net/~drchase/8013267/jdk.03/src/java.base/share/classes/java/lang/invoke/MemberName.java.html
> 
> 1047         private MemberName resolve(byte refKind, MemberName ref, Class<?> lookupClass) {
> 1048             MemberName m = ref.clone();  // JVM will side-effect the ref
> 1049             assert(refKind == m.getReferenceKind());
> 1050             try {
> 1051                 m = MethodHandleNatives.resolve(m, lookupClass);
> 1052                 m.checkForTypeAlias();
> 1053                 m.resolution = null;
> 1054                 m.intern();
> 
> Note that this performs an “intern” that ignores what is already found in the table
> and may not place m in the table if a duplicate name is already there. ?????????

That is:
 + m.intern();   // FIXME:  Here's where we stopped work the last time around.

I would try, first:
 + m = m.intern();

> testing:
> jtreg:
>  hotspot/{compiler,runtime,gc}
>  jdk/{vm,jdk,sun/{invoke,misc,reflect}
> by-hand: verified that the crash example for 8042235 still crash in an unmodified VM and does not crash  in a modified VM.
> 
> Modified and retested
> https://bugs.openjdk.java.net/secure/attachment/20184/RedefineMethodUsedByMultipleMethodHandles.java
> Old:
>        // Calling fooMH1.vmtarget crashes the VM
>        System.out.println("fooMH1.invoke = " + fooMH1.invokeExact());
> 
> New:
>        // Calling fooMH1.vmtarget crashes the VM
>        System.out.println("fooMH1.invoke = " + fooMH1.invokeExact());
>        System.out.println("fooMH2.invoke = " + fooMH2.invokeExact());
> 
> This turned out to be a reasonable thing to do, since the added line caused a crash till further corrections were made.
> (See fixes 2 and 3 above).
> 
> Should RedefineMethodUsedByMultipleMethodHandles.java also be added as a test?

I would say yes, though I'd defer to an SQE or Serguei on that.

> Are there other tests that would be good to run ?(and if they are ute tests, the more
> detailed the instructions for how to run them, the better.)

(Same deference.)

— John