Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8230501: Class data support for hidden classes #1171

Closed
wants to merge 25 commits into from

Conversation

mlchung
Copy link
Member

@mlchung mlchung commented Nov 11, 2020

Provide the Lookup::defineHiddenClassWithClassData API that allows live objects
be shared between a hidden class and other classes. A hidden class can load
these live objects as dynamically-computed constants via this API.

Specdiff
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8230501/specdiff/overview-summary.html

With this class data support and hidden classes, sun.misc.Unsafe::defineAnonymousClass
will be deprecated for removal. Existing libraries should replace their
calls to sun.misc.Unsafe::defineAnonymousClass with Lookup::defineHiddenClass
or Lookup::defineHiddenClassWithClassData.

This patch also updates the implementation of lambda meta factory and
MemoryAccessVarHandleGenerator to use class data. No performance difference
observed in the jdk.incubator.foreign microbenchmarks. A side note:
MemoryAccessVarHandleGenerator is removed in the upcoming integration of
JDK-8254162 but it helps validating the class data support.

Background

This is an enhancement following up JEP 371: Hidden Classes w.r.t.
"Constant-pool patching" in the "Risks and Assumption" section.

A VM-anonymous class can be defined with its constant-pool entries already
resolved to concrete values. This allows critical constants to be shared
between a VM-anonymous class and the language runtime that defines it, and
between multiple VM-anonymous classes. For example, a language runtime will
often have MethodHandle objects in its address space that would be useful
to newly-defined VM-anonymous classes. Instead of the runtime serializing
the objects to constant-pool entries in VM-anonymous classes and then
generating bytecode in those classes to laboriously ldc the entries,
the runtime can simply supply Unsafe::defineAnonymousClass with references
to its live objects. The relevant constant-pool entries in the newly-defined
VM-anonymous class are pre-linked to those objects, improving performance
and reducing footprint. In addition, this allows VM-anonymous classes to
refer to each other: Constant-pool entries in a class file are based on names.
They thus cannot refer to nameless VM-anonymous classes. A language runtime can,
however, easily track the live Class objects for its VM-anonymous classes and
supply them to Unsafe::defineAnonymousClass, thus pre-linking the new class's
constant pool entries to other VM-anonymous classes.

This extends the hidden classes to allow live objects to be injected
in a hidden class and loaded them via condy.

Details

A new Lookup::defineHiddenClassWithClassData API takes additional
classData argument compared to Lookup::defineHiddenClass.
Class data can be method handles, lookup objects, arbitrary user objects
or collections of all of the above.

This method behaves as if calling Lookup::defineHiddenClass to define
a hidden class with a private static unnamed field that is initialized
with classData at the first instruction of the class initializer.

MethodHandles::classData(Lookup lookup, String name, Class<?> type) and
MethodHandles::classDataAt(Lookup lookup, String name, Class<?> type, int index)
are the bootstrap methods to load the class data of the given lookup's lookup class.
The hidden class will be initialized when classData method is called if
the hidden class has not been initialized.

For a class data containing more than one single element, libraries can
create their convenience method to load a single live object via condy.

Frameworks sometimes want to dynamically create a hidden class (HC) and add it
it the lookup class nest and have HC to carry secrets hidden from that nest.
In this case, frameworks should not to use private static finals (in the HCs
they spin) to hold secrets because a nestmate of HC may obtain access to
such a private static final and observe the framework's secret. It should use
condy. In addition, we need to differentiate if a lookup object is created from
the original lookup class or created from teleporting e.g. Lookup::in
and MethodHandles::privateLookupIn.

This proposes to add a new ORIGINAL bit that is only set if the lookup
object is created by MethodHandles::lookup or by bootstrap method invocation.
The operations only apply to a Lookup object with original access are:

  • create method handles for caller-sensitve methods
  • obtain class data associated with the lookup class

No change to Lookup::hasFullPrivilegeAccess and Lookup::toString which
ignores the ORIGINAL bit.

Compatibility Risks

Lookup::lookupModes includes a new ORIGINAL bit. Most lookup operations
ignore this original bit except creating method handles for caller-sensitive methods
that expects the lookup from the original lookup class. Existing code compares
the return value of lookupModes to be a fixed value may be impacted. However
existing client has no need to expect a fixed value of lookup modes.
The incompatibility risk of this spec change is low.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1171/head:pull/1171
$ git checkout pull/1171

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 11, 2020

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 11, 2020
@openjdk
Copy link

openjdk bot commented Nov 11, 2020

@mlchung The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 11, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 11, 2020

Webrevs

@mlbridge
Copy link

mlbridge bot commented Nov 11, 2020

Mailing list message from Remi Forax on hotspot-compiler-dev:

Hi Mandy,
maybe a stupid question but why this mechanism is limited to hidden classes ?

regards,
R?mi

----- Mail original -----

De: "Mandy Chung" <mchung at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "hotspot compiler" <hotspot-compiler-dev at openjdk.java.net>
Envoy?: Mercredi 11 Novembre 2020 19:57:04
Objet: RFR: 8230501: Class data support for hidden classes

@mlchung
Copy link
Member Author

mlchung commented Nov 11, 2020

Hi Remi,

One primary motivation of the class data support is to allow hidden classes to refer each other as they are nameless and cannot be referenced in the constant-pool entries. I see the benefit of extending class data support to normal classes that can simplify the bytecode generated with fewer CP entries. I want to keep the scope just as a follow up to JEP 371 Hidden Classes so that we can deprecate Unsafe::defineAnonymousClass for removal. Soon we can remove the VM anonymous and constant pool patching code from HotSpot VM.

@mlchung
Copy link
Member Author

mlchung commented Nov 11, 2020

/label remove hotspot-compiler

@openjdk openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 11, 2020
@openjdk
Copy link

openjdk bot commented Nov 11, 2020

@mlchung
The hotspot-compiler label was successfully removed.

Copy link
Member

@JornVernee JornVernee left a comment

I took a look, since I'm interested in this API. Though, I can't really give a meaningful review for the access checking & ORIGINAL code changes.

@JornVernee
Copy link
Member

JornVernee commented Nov 12, 2020

Also (as mentioned offline), while it's possible with the current API to provide an Object[] as class data, and then load elements of that array into CP entries using downstream condys (e.g. by creating an array access var handle and then using that), I think it would be good to add a getArrayElement BSM for doing that to ConstantBootstraps to make this (seemingly) common case easier (maybe also a getField for loading instance fields). This would help to address the case where multiple live constants need to be injected.

I can file some RFEs, and would be happy to work on that.

@mlchung
Copy link
Member Author

mlchung commented Nov 13, 2020

Also (as mentioned offline), while it's possible with the current API to provide an Object[] as class data, and then load elements of that array into CP entries using downstream condys (e.g. by creating an array access var handle and then using that), I think it would be good to add a getArrayElement BSM for doing that to ConstantBootstraps to make this (seemingly) common case easier (maybe also a getField for loading instance fields). This would help to address the case where multiple live constants need to be injected.

I am uncomfortable with adding ConstantBootstraps::getArrayElement because an array is modifiable and not true constant. A final instance field can be modified via reflection and therefore it's not trusted as a constant either.

@JornVernee
Copy link
Member

JornVernee commented Nov 13, 2020

Also (as mentioned offline), while it's possible with the current API to provide an Object[] as class data, and then load elements of that array into CP entries using downstream condys (e.g. by creating an array access var handle and then using that), I think it would be good to add a getArrayElement BSM for doing that to ConstantBootstraps to make this (seemingly) common case easier (maybe also a getField for loading instance fields). This would help to address the case where multiple live constants need to be injected.

I am uncomfortable with adding ConstantBootstraps::getArrayElement because an array is modifiable and not true constant. A final instance field can be modified via reflection and therefore it's not trusted as a constant either.

I guess it would be similar to doing something like:

private static final Object[] arr = getArray();
private static final Object o = arr[0];

Setting arr[0] = new Object(); would not affect o after it has been initialized. The difference being that a constant for the array element would be resolved (more) lazily, so it would be possible to resolve the array, then modify it, and then resolve the constant that loads the element, which would see the updated value as well.

However, we can already store an array in the constant pool today, and modify it if we want, even though, as you say, it is mutable. In that sense getArrayElement doesn't introduce anything new.

Mutating the array is probably a bad idea though, so maybe we want to push users away from using arrays? To prevent inadvertent modification, maybe an immutable List should be used in place of a plain array. In that case, would you feel differently about added a getListElement BSM, that gets an element from an (immutable) List instead?

Another idea is to rely on records, since they can not be mutated with reflection at all. i.e. we add a getRecordComponent BSM instead, so a class data Object can be a record, and then the BSM can be used to extract individual components.

WDYT?

@JornVernee
Copy link
Member

JornVernee commented Nov 13, 2020

Sorry, this is unrelated to this RFR, I will start a separate discussion thread elsewhere.

Copy link
Member

@PaulSandoz PaulSandoz left a comment

IIUC classData can be used for an original lookup that is not produced by the result of defineHiddenClassWithClassData, but in such cases the class data will always be null.

Since defineHiddenClassWithClassData rejects null values for class data, we could detect such usage and throw in the bootstrap methods. That would require a special constant assignment for hidden classes with no class data. Probably not worth it.

Recommend an API note.

return BootstrapMethodInvoker.widenAndCast(classdata, type);
} catch (ClassCastException e) {
throw e;
} catch (Throwable e) {
Copy link
Member

@PaulSandoz PaulSandoz Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following might be more appropriate so, in general, errors and runtime exceptions are not explicitly wrapped:

        try {
            return BootstrapMethodInvoker.widenAndCast(classdata, type);
        }
        catch (RuntimeException | Error e) {
            throw e;
        }
        catch (Throwable e) {
            throw new InternalError("Unexpected exception", e);
        }

same applies to classDataAt and ConstantBootstraps.explicitCast. Refinement of the runtime exceptions is also possible, but i think the key thing here is to let errors pass through and any possibly expected runtime exceptions will get wrapped in BootstrapMethodError.

Copy link
Member Author

@mlchung mlchung Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes a good refinement.

diff --git a/src/java.base/share/classes/java/lang/invoke/ConstantBootstraps.java b/src/java.base/share/classes/java/lang/invoke/ConstantBootstraps.java
index 71cae83e160..27d74284dc6 100644
--- a/src/java.base/share/classes/java/lang/invoke/ConstantBootstraps.java
+++ b/src/java.base/share/classes/java/lang/invoke/ConstantBootstraps.java
@@ -413,8 +413,8 @@ public final class ConstantBootstraps {
         MethodHandle conv = MethodHandles.explicitCastArguments(id, mt);
         try {
             return conv.invoke(value);
-        } catch (ClassCastException e) {
-            throw e; // specified, let CCE through
+        } catch (RuntimeException|Error e) {
+            throw e; // let specified CCE and other runtime exceptions/errors through
         } catch (Throwable throwable) {
             throw new InternalError(throwable); // Not specified, throw InternalError
         }
diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
index cd9bdbaf5a3..368948ab5a8 100644
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -337,8 +342,8 @@ public class MethodHandles {
 
          try {
              return BootstrapMethodInvoker.widenAndCast(classdata, type);
-         } catch (ClassCastException e) {
-             throw e;
+         } catch (RuntimeException|Error e) {
+             throw e; // let CCE and other runtime exceptions through
          } catch (Throwable e) {
              throw new InternalError(e);
          }
@@ -409,8 +414,8 @@ public class MethodHandles {
         try {
             Object element = classdata.get(index);
             return BootstrapMethodInvoker.widenAndCast(element, type);
-        } catch (ClassCastException|NullPointerException|IndexOutOfBoundsException e) {
-            throw e;
+        } catch (RuntimeException|Error e) {
+            throw e; // let specified exceptions and other runtime exceptions/errors through
         } catch (Throwable e) {
             throw new InternalError(e);
         }

* A lookup class with no class data.
*/
@Test
public void noClassData() throws Throwable {
Copy link
Member

@PaulSandoz PaulSandoz Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throws Throwable needed on this and other method declarations?

Copy link
Member Author

@mlchung mlchung Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noClassData only needs IllegalACcessException. assertClassData throws Throwable because it unwraps from InvocationTargetException. I can take a pass to clean this up further.

@openjdk
Copy link

openjdk bot commented Nov 18, 2020

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8230501: Class data support for hidden classes

Reviewed-by: jvernee, psandoz, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

  • 11dad14: 8257445: (zipfs) Add DataProvider to TestLocOffsetFromZip64EF.java
  • 29f86e0: 8256536: Newer AMD 19h (EPYC) Processor family defaults
  • 7f58a8e: 8213719: Both sect163r2 and sect163k1 are default curves for field size 163
  • ae5b526: 8257448: Clean duplicated non-null check in the SunJSSE provider implementation

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 18, 2020
@mlchung
Copy link
Member Author

mlchung commented Nov 18, 2020

IIUC classData can be used for an original lookup that is not produced by the result of defineHiddenClassWithClassData, but in such cases the class data will always be null.

Yes that's the case. I see some clarification would help. I added the following in the spec of classData and classDataAt:

     * <p> A hidden class created by {@link Lookup#defineHiddenClass(byte[], boolean, Lookup.ClassOption...)
     * Lookup::defineHiddenClass} and non-hidden classes have no class data.
     * {@code null} is returned if this method is called on the lookup object
     * on these classes.

@mlchung
Copy link
Member Author

mlchung commented Nov 18, 2020

Copy link
Member

@JornVernee JornVernee left a comment

Left 2 minor comments on the new additions in line.

* Care should be taken w.r.t. mutability for example when passing
* an array or other mutable structure through the class data.
Copy link
Member

@JornVernee JornVernee Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessarily clear why/how care should be taken from this text. I suggest:

Suggested change
* Care should be taken w.r.t. mutability for example when passing
* an array or other mutable structure through the class data.
* Care should be taken w.r.t. mutability for example when passing
* an array or other mutable structure through the class data. Such
* a constant should not be mutated, as downstream consumers of
* this constant, such as other constants, are not guaranteed to see
* the updated value, depending on the timing of their resolution.

Copy link
Member Author

@mlchung mlchung Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -2156,8 +2156,10 @@ public class MethodHandles {
          * (unlike private static fields that are accessible to nestmates).
          * Care should be taken w.r.t. mutability for example when passing
          * an array or other mutable structure through the class data.
-         * If you use a {@code List}, it is a good practice to make it unmodifiable
-         * for example via {@link List#of List::of}.
+         * Changing any value stored at the class data at runtime may lead to
+         * unpredictable behavior.
+         * If the class data is a {@code List}, it is a good practice to make it
+         * unmodifiable for example via {@link List#of List::of}.
          *
          * @param bytes     the class bytes
          * @param classData pre-initialized class data

Copy link
Member

@JornVernee JornVernee Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

* If you use a {@code List}, it is a good practice to make it unmodifiable
* for example via {@link List#of List::of}.
Copy link
Member

@JornVernee JornVernee Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formal language.

Suggested change
* If you use a {@code List}, it is a good practice to make it unmodifiable
* for example via {@link List#of List::of}.
* If a {@code List} is used, it is a good practice to make it unmodifiable
* for example via {@link List#of List::of}.

@ChrisHegarty
Copy link
Member

ChrisHegarty commented Nov 23, 2020

It is my understanding that Lookup object returned from defineHiddenClass[WithClassData] features the ORIGINAL access lookup mode, right? I cannot find a normative statement to confirm this. If it is the case, then it would be good to clarify this.

@mlchung
Copy link
Member Author

mlchung commented Nov 23, 2020

It is my understanding that Lookup object returned from defineHiddenClass[WithClassData] features the ORIGINAL access lookup mode, right? I cannot find a normative statement to confirm this. If it is the case, then it would be good to clarify this.

Yes with ORIGINAL access. I updated @return for both defineHiddenClass and defineHiddenClassWithClassData as follows:

-         * @return the {@code Lookup} object on the hidden class
+         * @return the {@code Lookup} object on the hidden class with
+         * {@linkplain #ORIGINAL original} and
+         * {@linkplain Lookup#hasFullPrivilegeAccess() full privilege} access

@ChrisHegarty
Copy link
Member

ChrisHegarty commented Nov 24, 2020

It is my understanding that Lookup object returned from defineHiddenClass[WithClassData] features the ORIGINAL access lookup mode, right? I cannot find a normative statement to confirm this. If it is the case, then it would be good to clarify this.

Yes with ORIGINAL access. I updated @return for both defineHiddenClass and defineHiddenClassWithClassData as follows:

-         * @return the {@code Lookup} object on the hidden class
+         * @return the {@code Lookup} object on the hidden class with
+         * {@linkplain #ORIGINAL original} and
+         * {@linkplain Lookup#hasFullPrivilegeAccess() full privilege} access

Thank you. This addresses my concern.

@mlchung
Copy link
Member Author

mlchung commented Dec 1, 2020

/integrate

@openjdk openjdk bot closed this Dec 1, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 1, 2020
@openjdk
Copy link

openjdk bot commented Dec 1, 2020

@mlchung Since your change was applied there have been 4 commits pushed to the master branch:

  • 11dad14: 8257445: (zipfs) Add DataProvider to TestLocOffsetFromZip64EF.java
  • 29f86e0: 8256536: Newer AMD 19h (EPYC) Processor family defaults
  • 7f58a8e: 8213719: Both sect163r2 and sect163k1 are default curves for field size 163
  • ae5b526: 8257448: Clean duplicated non-null check in the SunJSSE provider implementation

Your commit was automatically rebased without conflicts.

Pushed as commit 4356469.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mlchung mlchung deleted the class-data branch May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants