On Thu, 12 Nov 2020 14:18:17 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> 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)` >> is a bootstrap method 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. >> We can reconsider if such a convenience method is needed in the future. >> >> 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. > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 552: > >> 550: Handle classDataBsm = new Handle(H_INVOKESTATIC, >> Type.getInternalName(MethodHandles.class), "classData", >> 551: >> classDataMType.descriptorString(), false); >> 552: ConstantDynamic implMethodCondy = new >> ConstantDynamic("implMethod", MethodHandle.class.descriptorString(), >> classDataBsm); > > Could be moved to clinit? Good suggestion. Thanks. > test/jdk/java/lang/invoke/AccessControlTest.java line 183: > >> 181: * then no members, not even public members, will be accessible >> 182: * i.e. all access modes are lost. >> 183: * [A8] If the new lookup class, the old lookup class and the >> previous lookup class > > Seems like a typo? There are 2 A8s now, and no A7, it seems. Yes, it's a typo. Fixed. > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 319: > >> 317: */ >> 318: public static <T> T classData(Lookup caller, String name, Class<T> >> type) throws IllegalAccessException { >> 319: Objects.requireNonNull(name); > > `name` is unused, but it can not be `null`. I think it would be good to add > something about that to the javadoc. e.g. > * @param name ignored (must not be {@code null}) > > FWIW, `classDataAt` doesn't check this. Should `caller` be checked as well? Good catch. `name` argument is unused and should not be checked. I added `@throws NullPointerException if {@code caller} or {@code type} argument is {@code null}`. `caller.lookupModes()` will throw NPE if it's null. > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 403: > >> 401: >> 402: @SuppressWarnings("unchecked") >> 403: List<T> classData = List.class.cast(classdata); > > Couldn't this call `classData` ? > Suggestion: > > @SuppressWarnings("unchecked") > List<T> classData = classData(caller, name, List.class); > if (classData == null) return null; Fixed. > test/jdk/java/lang/invoke/MethodHandles/classData/ClassDataTest.java line 78: > >> 76: @Test >> 77: public void noClassData() throws Throwable { >> 78: assertTrue(MethodHandles.classData(LOOKUP, "dummy", >> Object.class) == null); > > Could be: > Suggestion: > > assertNull(MethodHandles.classData(LOOKUP, "dummy", Object.class)); > > FWIW OK. ------------- PR: https://git.openjdk.java.net/jdk/pull/1171