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

[Christian Thalinger]

There are a couple problems that haven’t been addressed yet.  One of which is the fact that:
+     public <E extends Comparable<? super E>> E internMemberName(E memberName) {
in java.lang.Class cannot be public.  You have to add some magic token machinery to make it usable from java.lang.invoke.MemberName (John knows more about this).  Also:
+         public <E extends Comparable<? super E>> E intern(E memberName) {
has some commented code which I used to make sure to not introduce the same performance regression we saw in the original implementation.  I can’t find the bug right now but maybe Serguei can give us the link.  We have to make sure that Arrays.binarySearch fits all array size cases.

> 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
> 
> The “bug” is a cleanup/space-saving RFE that enables fixes for other bugs related
> to method name tables — e.g., failure to properly update two method handles
> referring to the same method after the method is redefined.  The patch is months
> old and needed to be brought up to date.

Well, it is a real bug.

> 
> In addition, the patch needed the following changes beyond what was seen in
> other reviews:
> 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.

Yes, that was an oversight.

> 
> 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”).

Right, thanks.

> 
> 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.  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. ?????????

I can’t remember if that was intended or more a quick hack to get things working.  Looking at it again it seems wrong.

> 
> 
> 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?

Yes.

> 
> 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.)

You can run the tests that failed during nightly testing.

> 
> David
> 
> 
>