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

[serguei.spitsyn at oracle.com]

On 10/20/14 4:20 PM, Christian Thalinger wrote:
> 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.

The bug is:
   https://jbs.oracle.com/bugs/browse/JDK-8014288
     perf regression in nashorn JDK-8008448.js test after 8008511 changes

and the fix is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8014288-JVMTI-JSR292.1/

   The approach was to use the*InstanceKlass::idnum_allocated_count()* 
and *Method::method_idnum()*
   for organizing a direct indexing based access to the MNT elementsas 
in the example below:

  oop MethodHandles::init_method_MemberName(Handle mname, Method* m, bool do_dispatch,
                                            KlassHandle receiver_limit_h) {

      . . .

    m->method_holder()->add_member_name(*m->method_idnum()*, mname);

    return mname();
  }


Not sure, the above is helpful enough though. :)

Thanks,
Serguei

>
>> On Oct 17, 2014, at 11:16 AM, David Chase <david.r.chase at oracle.com 
>> <mailto: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/%7Edrchase/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 
>> <http://cr.openjdk.java.net/%7Edrchase/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
>>
>>
>>
>