JDK-8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames
  • Type: Enhancement
  • Component: core-libs
  • Sub-Component: java.lang.invoke
  • Affected Version: 9
  • Priority: P3
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2013-04-25
  • Updated: 2020-07-16
Related Reports
Duplicate :  
JDK-8042235 - redefining method used by multiple MethodHandles crashes VM
Duplicate :  
JDK-8042593 - redefining method used by multiple MethodHandles crashes VM
Duplicate :  
JDK-8040201 - JSR 292: old non-redefined code is entered again after JVMTI PopFrame followed by class redefinition in MethodHandle.invoke
Duplicate :  
JDK-8027162 - JSR292: implement MemberName interning
Duplicate :  
JDK-8042592 - redefining method used by multiple MethodHandles crashes VM
Relates :  
JDK-8166852 - JavaScript engine eval() method causes JVM Young GC to pauses seconds
Relates :  
JDK-8008511 - JSR 292: MemberName vmtarget refs to methods must be updated at class redefinition
Relates :  
JDK-8162795 - [REDO] MemberNameTable doesn't purge stale entries
Relates :  
JDK-8054315 - vm/mlvm/indy/func/jvmti/mergeCP_indy2none_a fails in nightly
Relates :  
JDK-8042235 - redefining method used by multiple MethodHandles crashes VM
Relates :  
JDK-8152271 - MemberNameTable doesn't purge stale entries
Relates :  
JDK-8174749 - Use hash table/oops for MemberName table
Description
The JSR 292 Lookup API makes native calls to obtain MemberNames which underly direct method handles.  To simplify MemberName lookup and reduce duplicate creation of MemberName, we will move the MemberName table created by JDK-8008511 into the Java heap, so that Lookup queries can refer to that.

See comments on JDK-8005232 for a related ReflectionData structure.  This structure would be extended to include the content of the MemberNameTable.  Because a SoftReference is used to make the ReflectionData structure droppable, we would place the new data fields in the SoftReference (making it into a ClassData subclass) rather adding them to ReflectionData.

Also, certain other fields in Class (such as those holding annotations) could be moved at the same time into the ClassData, for a net reduction of application footprint.
Comments
>> j.l.i.MemberName implements Cloneable and MN cloning is pervasively >> used during DirectMethodHandle construction (see MN.resolve() and >> other MN instance methods). > > So there'd still be a leak but just not a simple one. Are there plans > to not use cloning? That should be a separate issue, though. The VM > would now reuse MemberName with this change. No, I'm not aware of any plans to get rid of MN cloning. The following code takes care of cloned MN instance: --- old/src/share/vm/prims/jvm.cpp 2016-06-09 17:19:13.896240434 -0400 +++ new/src/share/vm/prims/jvm.cpp 2016-06-09 17:19:13.454960882 -0400 @@ -695,7 +695,7 @@ // This can safepoint and redefine method, so need both new_obj and method // in a handle, for two different reasons. new_obj can move, method can be // deleted if nothing is using it on the stack. - m->method_holder()->add_member_name(new_obj()); + m->method_holder()->add_member_name(new_obj(), false); } } The cloned instance is usually adjusted (changes in MN.flags) right away, so it becomes non-equal to the original one. It allows to bypass repeated resolution. When you talk about a leak, do you mean the situation when there's already a duplicate of adjusted MN registered ? If it causes a leak, we can do a call into the JVM to intern adjusted MN instance, e.g: jdk/src/java.base/share/classes/java/lang/invoke/MemberName.java: private MemberName changeReferenceKind(byte refKind, byte oldKind) { assert(getReferenceKind() == oldKind); assert(MethodHandleNatives.refKindIsValid(refKind)); flags += (((int)refKind - oldKind) << MN_REFERENCE_KIND_SHIFT); return MethodHandleNatives.intern(this); // returns interned MN instance } Best regards, Vladimir Ivanov
15-06-2016
The native runtime is going to need the MemberName table to swap Method* for class redefinition in the near and likely far future. It seems odd and awkward to move this one part out to Java. The MemberNames must be saved on a list internally in the JVM for this purpose because Method* is a native type. The runtime needs to find them. There is a performance problem with MemberNames in that they are stored as weak references inside the MemberName table. Weak references are costly to GC. If there were a MemberName cache that would prevent or alleviate duplicate MemberName creation, maybe they don't need to be jweak. Maybe they could be strong roots from InstanceKlass and not go away until class unloading, and that would be acceptable footprint wise. The reason I'm looking at this is in the context of the StackWalking API. Creating MemberNames for methods on the stack for Throwable is particularly expensive because they are weak references. I still think footprint-wise compared to what we not collect for stack frames in Throwable, MemberName are too expensive. Eliminating the weak references might be helpful though if it's possible.
21-01-2016
Retargeted to 8u60. It's too late for an enhancement to go into 8u40.
02-12-2014
Coleen P fixed this on the runtime side, so this is no longer a bug, now it is (potentially) an RFE pending a sanity check on performance for things like Nashorn startup. It may not even be that. The potential performance implications of the runtime-side fix is that a large number of weak references may be created if MemberNames are not cached on the Java side.
26-11-2014
Additional benchmarks (executive summary -- not slower, nominally a hair faster but well within error range): This was measured with an improved version of the code that separates the jdk lookup side from the jvm sharing. TestBenchmark Mode Samples Score ScoreError Units o.m.b.s.o.Box2DBench.test ss 10 10727.162 439.533 ms o.m.b.s.o.CodeLoadBench.test ss 10 7183.338 518.556 ms o.m.b.s.o.CryptoBench.test ss 10 1988.185 81.742 ms o.m.b.s.o.DeltaBlueBench.test ss 10 1183490.099 56002.026 us o.m.b.s.o.EarleyBoyerBench.test ss 10 4015.816 73.154 ms o.m.b.s.o.GbemuBench.test ss 10 11368.880 680.575 ms o.m.b.s.o.MandreelBench.test ss 10 21796.891 1303.858 ms o.m.b.s.o.NavierStokesBench.test ss 10 1549.187 99.732 ms o.m.b.s.o.PdfJSBench.test ss 10 7109.783 530.755 ms o.m.b.s.o.RaytraceBench.test ss 10 2535794.621 76258.581 us o.m.b.s.o.RegexpBench.test ss 10 497.817 69.301 ms o.m.b.s.o.RichardsBench.test ss 10 765404.725 30938.084 us o.m.b.s.o.SplayBench.test ss 10 404185.278 30837.102 us o.m.b.s.o.TypescriptBench.test ss 10 81.548 1.963 s RefBenchmark Mode Samples Score ScoreError Units o.m.b.s.o.Box2DBench.test ss 10 10830.884 395.274 ms o.m.b.s.o.CodeLoadBench.test ss 10 7152.992 528.807 ms o.m.b.s.o.CryptoBench.test ss 10 1942.657 47.388 ms o.m.b.s.o.DeltaBlueBench.test ss 10 1196692.491 45619.538 us o.m.b.s.o.EarleyBoyerBench.test ss 10 4038.578 165.961 ms o.m.b.s.o.GbemuBench.test ss 10 11834.395 660.828 ms o.m.b.s.o.MandreelBench.test ss 10 21917.193 1061.759 ms o.m.b.s.o.NavierStokesBench.test ss 10 1525.967 31.768 ms o.m.b.s.o.PdfJSBench.test ss 10 7161.232 617.921 ms o.m.b.s.o.RaytraceBench.test ss 10 2575299.700 96098.340 us o.m.b.s.o.RegexpBench.test ss 10 527.140 20.796 ms o.m.b.s.o.RichardsBench.test ss 10 768785.607 35806.298 us o.m.b.s.o.SplayBench.test ss 10 426250.663 48720.881 us o.m.b.s.o.TypescriptBench.test ss 10 82.558 1.911 s Comparison test/ref scoreError as fraction o.m.b.s.o.Box2DBench.test 0.99 0.04 o.m.b.s.o.CodeLoadBench.test 1.00 0.07 o.m.b.s.o.CryptoBench.test 1.02 0.02 o.m.b.s.o.DeltaBlueBench.test 0.99 0.04 o.m.b.s.o.EarleyBoyerBench.test 0.99 0.04 o.m.b.s.o.GbemuBench.test 0.96 0.06 o.m.b.s.o.MandreelBench.test 0.99 0.05 o.m.b.s.o.NavierStokesBench.test 1.02 0.02 o.m.b.s.o.PdfJSBench.test 0.99 0.09 o.m.b.s.o.RaytraceBench.test 0.98 0.04 o.m.b.s.o.RegexpBench.test 0.94 0.04 o.m.b.s.o.RichardsBench.test 1.00 0.05 o.m.b.s.o.SplayBench.test 0.95 0.11 o.m.b.s.o.TypescriptBench.test 0.99 0.02 Geometric mean 0.99
07-11-2014
Trying to get some CR matching to work with nightly build RULE vm/mlvm/indy/func/jvmti/mergeCP_indy2manyDiff_b Exception java.lang.RuntimeException RULE vm/mlvm/indy/func/jvmti/mergeCP_indy2manyDiff_b Exception java.lang.reflect.InvocationTargetException RULE vm/mlvm/indy/func/jvmti/mergeCP_indy2manyDiff_b ExitCode 1
04-11-2014
Preliminary benchmarking results -- this using a version of the patch in which the jdk/jvm shared data is an always-sorted array of MemberName, with plenty of barriers in the update code to be very very carefully safe. 1) From https://bugs.openjdk.java.net/browse/JDK-8014288?focusedCommentId=13325265&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13325265 $ time $JAVA_HOME/bin/java -showversion -Dtest.js.roots=test/script/basic -Dtest.js.includes="JDK-8008448.js" -Djava.ext.dirs=dist/ -cp test/lib/testng.jar:build/test/classes/ org.testng.TestNG -testclass build/test/classes/jdk/nashorn/internal/test/framework/ScriptTest.class comparing jdk9 reference and test, no apparent difference in 10 runs each: real times (on an 18-processor 64-bit Linux box): REF TEST median 5.594 5.575 average 5.607 5.532 min 5.443 5.200 user times: REF TEST median 22.186 22.250 average 22.139 22.386 min 20.563 21.278 2) From https://bugs.openjdk.java.net/browse/JDK-8014288 5 runs of reference all took 4 minutes and single-digit seconds 9 out of 10 runs of test all took 4 minutes and single-digit seconds, with one 5-minute outlier (???)
04-11-2014
There's multiple calls to intern, not just that one constructor. What's the plan here? private MemberName resolve(byte refKind, MemberName ref, Class<?> lookupClass) { MemberName m = ref.clone(); // JVM will side-effect the ref assert(refKind == m.getReferenceKind()); try { m = MethodHandleNatives.resolve(m, lookupClass); m.checkForTypeAlias(); m.resolution = null; m.intern(); Cloning, side-effecting, and interning, yet dropping the result of the intern on the floor (what if it is already there? What if someone redefines methods?)
16-10-2014
I think I see a problem. We want to intern MemberNames so that we don't have duplicates. However, in a PUBLIC CONSTRUCTOR, we create a duplicate, and at the end, we "intern()" it to check for duplicates, but drop whatever we found on the floor and return the duplicate anyhow: /** Create a name for the given reflected constructor. The resulting name will be in a resolved state. */ @SuppressWarnings("LeakingThisInConstructor") public MemberName(Constructor<?> ctor) { ctor.getClass(); // NPE check // fill in vmtarget, vmindex while we have ctor in hand: MethodHandleNatives.init(this, ctor); assert(isResolved() && this.clazz != null); this.name = CONSTRUCTOR_NAME; if (this.type == null) { this.type = new Object[] { void.class, ctor.getParameterTypes() }; } // Unreflected member names are resolved so intern them here. this.intern(); } If this weren't a public constructor I would replace it with a factory and I would be done, but since it is not, I am not. AHA -- MemberName is not itself public, so I guess this is possible. Is this in conformance with the Grand Plan?
16-10-2014
Since MemberName is not a public class and this constructor is only used from two places in the java.lang.invoke package, then your factory solution should be ok.
16-10-2014
I got through this much testing: testing: jtreg hotspot/{compiler,runtime,gc} jtreg jdk/{vm,jdk,sun/{invoke,misc,reflect} by-hand: verified that the crash example for 8042235 still crashes in an unmodified VM and does not crash in a modified VM. Candidate diffs are now at http://cr.openjdk.java.net/~drchase/8013267/ HOWEVER: A trivial and obvious modification of that test case still crashes, but it crashes one line later. See 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()); // ���Fix��� moves crash to this line: System.out.println("fooMH2.invoke = " + fooMH2.invokeExact()); I'm looking into this, I thought I was getting close to a push but then curiosity got the best of me.
14-10-2014
If the classData is null, does that mean that this step could just be skipped? I would take this to mean that nobody accessed the classData, therefore nothing is there, therefore nothing (there) is obsolete. I tried that, and so far, so good, will rerun tests and see what I get.
13-10-2014
Revived the patch (there was a wrong type on a field -- array of Object instead of array of Comparable), is currently failing these hotspot tests: TEST: compiler/codecache/CheckUpperLimit.java (also fails in unpatched) These five all fail at # Internal Error (/home/drchase/work/jdk9/hotspot/src/share/vm/classfile/javaClasses.cpp:939), ... # assert(is_instance(obj)) failed: null TEST: compiler/profiling/spectrapredefineclass/Launcher.java TEST: compiler/profiling/spectrapredefineclass_classloaders/Launcher.java TEST: runtime/RedefineTests/RedefineRunningMethods.java TEST: runtime/RedefineTests/RedefineFinalizer.java TEST: runtime/RedefineObject/TestRedefineObject.java The failure occurs from this call at 2791 -- class_data is null. 2785 void InstanceKlass::adjust_member_name_table(Method** old_methods, Method** new_methods, 2786 int methods_length, bool *trace_name_printed) { 2787 assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint"); 2788 2789 // Dig out the member name table array. 2790 oop class_data = java_lang_Class::classData(java_mirror()); 2791 objArrayOop array = java_lang_Class_ClassData::elementData(class_data);
13-10-2014
SQE is ok to defer from 8u20.
25-06-2014
8u20-defer-request justification: This change cannot be pushed until JPRT is able to handle depending HotSpot and JDK changes to go together. JPRT is close to be able to do that but it's likely too late for 8u20. The other option would be to have a flag day.
24-06-2014
As an FYI, we cannot push this change before JPRT is able to handle depending HotSpot and JDK changes to go together.
27-05-2014
Sorry, I meant the native code can't change them structurally (e.g. re-allocate/resize an array). They only thing the native code should do is exactly the updating of the Method* values.
05-05-2014
No, the native code will adjust the entries by changing them to the new Method*.
05-05-2014
Make sense. As long as the native code treats the data structure as read-only, then that should work.
03-05-2014
I think the idea is to have the MemberName table in Java (presumably as an array to keep it simple) and let MemberNameTable::adjust_method_entries adjust the entries. There is no need to up-call to Java if the data structure is simple and we can easily walk it.
02-05-2014
There are currently 11 fields in java.lang.Class, annotationData annotationType, cachedConstructor, classRedefinedCount, classValueMap, enumConstantDirectory enumConstant genericInfo name, newInstanceCallerCache reflectionData what's John propose is to try to group them in 3 categories, the ones that are never null (like name or classRedefinedCount), the ones that are null by default and can be dropped if low on memory and the ones that are null by default but should not be dropped. The ones of the first category should be field of Class, the one of the second category should be stored in a soft reference, the ones of the third category should be field of the soft reference (stored as field of a class that inherits from SoftReference). so java.lang.Class should be something like: class Class { String name; // category 1 int classRedefinedCount; ClassData data; static class Data extends SoftReference<ReflectionData> { ClassValueMap classValueMap // category 3 ... } static class ReflectionData { ... // category 2 } } cheers, R��mi
01-05-2014
Unless you make javaClasses.cpp aware of member name table? That's more unreliable C++ code. You can't upcall to Java to adjust them because you are at a safepoint.
01-05-2014
John, when you say ClassData are you talking about ClassValue or something different?
01-05-2014
If MemberNameTable is moved to Java code, how will class redefinition adjust the Method* values in MemberName.vmtarget? This is currently done in MemberNameTable::adjust_method_entries and must be called on the VM thread (or at least be at a safepoint). One solution would be to use a separate Java wrapper for Method* values (e.g. VMMethod) and have those managed by a native VMMethodTable associated with each InstanceKlass.
01-05-2014