Re: RFR: 8230501: Class data support for hidden classes [v2]
On Fri, 13 Nov 2020 18:42:53 GMT, Mandy Chung wrote: > > 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? - PR: https://git.openjdk.java.net/jdk/pull/1171
Re: RFR: 8230501: Class data support for hidden classes [v2]
On Fri, 13 Nov 2020 19:23:54 GMT, Jorn Vernee wrote: >>> 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. > >> > 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? Sorry, this is unrelated to this RFR, I will start a separate discussion thread elsewhere. - PR: https://git.openjdk.java.net/jdk/pull/1171
Re: RFR: 8230501: Class data support for hidden classes [v2]
On Thu, 12 Nov 2020 15:19:30 GMT, Jorn Vernee wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/1171
Re: RFR: 8230501: Class data support for hidden classes [v2]
> 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 `loo
