RFR 8186046 Minimal ConstantDynamic support
karen.kinnear at oracle.com
Fri Nov 3 18:14:53 UTC 2017
Thank you so much for doing this jointly.
Couple of questions/comments:
1. thank you for making UseBootstrapCallInfo diagnostic
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?
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;”
ofBasicType: case T_OBJECT return constantTag(JVM_CONSTANT_String) ?
why not JVM_CONSTANT_CLASS?
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?
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
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?
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 did I read the invoke dynamic method incorrectly?
> On Oct 26, 2017, at 1:03 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 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.
More information about the core-libs-dev