On Wed, 11 Nov 2020 18:52:10 GMT, Mandy Chung <mch...@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. 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. 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? 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. 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? 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; 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 ------------- PR: https://git.openjdk.java.net/jdk/pull/1171
