Thank you so much for doing this jointly. Couple of questions/comments: 1. thank you for making UseBootstrapCallInfo diagnostic
2. org/objectweb/asmClassReader.java why hardcoded 17 instead of ClassWriter.CONDY? 3. java/lang/invoke/package-info.java 128-134 Error handling could be clearer. My understanding is that if a LinkageError or subclass is thrown, this will be rethrown for all subsequent attempts. Other errors, e.g. VMError may retry resolution Also: after line 165: rules do not enable the JVM to share call sites. Is it worth noting anywhere that the Constant_Dynamic resolution is shared? 4. SD::find_java_mirror_for_type lines 2679+ ConDy should not be supporting CONSTANT_Class(“;” + FD) IIRC that is temporary for MVT but not part of ConDy’s spec, nor part of what should be checked in for 18.3 also line 2731 - remove comment about “Foo;” 5. constantTag.hpp ofBasicType: case T_OBJECT return constantTag(JVM_CONSTANT_String) ? why not JVM_CONSTANT_CLASS? 6. SD::find_java_mirror_for_type You have resolve_or_null/fail doing CHECK_(empty) which should check for a NULL constant_type_klass. This is followed by an explicit if (constant_type_klass == NULL) — is that needed? 7. reflection.cpp get_mirror_from_signature Looks like potential for side effects. Odd to set mirror_oop inside if (log_is_enabled) Is the intent to assert that k->java_mirror() == mirror_oop? Or is the issue that we have a make drop here and the potential for a safe point? If so, make a handle and extract it on return? — Or better yet - don’t make any changes to reflection.cpp - not necessary 8. BootstrapMethodInvoker Could you possibly add a comment that the default today is vmIsPushing and IsPullModeBSM is false? Or find a slightly different naming to make that clearer? 9. test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap method that was not ACC_STATIC? Or was not ACC_PUBLIC? Or was Or did I read the invoke dynamic method incorrectly? thanks, Karen > On Oct 26, 2017, at 1:03 PM, Paul Sandoz <paul.san...@oracle.com> wrote: > > Hi, > > Please review the following patch for minimal dynamic constant support: > > > http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/ > > <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/> > > https://bugs.openjdk.java.net/browse/JDK-8186046 > <https://bugs.openjdk.java.net/browse/JDK-8186046> > https://bugs.openjdk.java.net/browse/JDK-8186209 > <https://bugs.openjdk.java.net/browse/JDK-8186209> > > This patch is based on the JDK 10 unified HotSpot repository. Testing so far > looks good. > > By minimal i mean just the support in the runtime for a dynamic constant pool > entry to be referenced by a LDC instruction or a bootstrap method argument. > Much of the work leverages the foundations built by invoke dynamic but is > arguably simpler since resolution is less complex. > > A small set of bootstrap methods will be proposed as a follow on issue for 10 > (these are currently being refined in the amber repository). > > Bootstrap method invocation has not changed (and the rules are the same for > dynamic constants and indy). It is planned to enhance this in a further major > release to support lazy resolution of bootstrap method arguments. > > The CSR for the VM specification is here: > > https://bugs.openjdk.java.net/browse/JDK-8189199 > <https://bugs.openjdk.java.net/browse/JDK-8189199> > > the j.l.invoke package documentation was also updated but please consider the > VM specification as the definitive "source of truth" (we may clean up this > area further later on so it becomes more informative, and that may also apply > to duplicative text on MethodHandles/VarHandles). > > Any AoT-related work will be deferred to a future release. > > — > > This patch only supports x64 platforms. There is a small set of changes > specific to x64 (specifically to support null and primitives constants, as > prior to this patch null was used as a sentinel for resolution and certain > primitives types would never have been encountered, such as say byte). > > We will need to follow up with the SPARC platform and it is hoped/anticipated > that OpenJDK members responsible for other platforms (namely ARM and PPC) > will separately provide patches. > > — > > Many of tests rely on an experimental byte code API that supports the > generation of byte code with dynamic constants. > > One test uses class file bytes produced from a modified version of asmtools. > The modifications have now been pushed but a new version of asmtools need to > be rolled into jtreg before the test can operate directly on asmtools > information rather than embedding class file bytes directly in the test. > > — > > Paul.
