Hi Mndy, Looks good, I don't like too much the fact that there is a new boolean field in j.l.r.Field instead of using the modifiers like in the VM counterpart, but it will require masking and unmasking the modifiers which is a far bigger change, too big to worth it in my opinion.
So +1 Rémi ----- Mail original ----- > De: "mandy chung" <mandy.ch...@oracle.com> > À: "hotspot-dev" <hotspot-...@openjdk.java.net>, "core-libs-dev" > <core-libs-dev@openjdk.java.net>, "amber-dev" > <amber-...@openjdk.java.net> > Envoyé: Lundi 15 Juin 2020 23:58:19 > Objet: (15) RFR: JDK-8247444: Trust final fields in records > This patch is joint contribution from Christoph Dreis [1] and me. > > Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/ > CSR: https://bugs.openjdk.java.net/browse/JDK-8247517 > > This proposes to make final fields in records notmodifiable via > reflection. Field::set throws IAE if a Field is not modifiable. > Thecurrent specification specifies the following Fields not modifiable: > - static final fields in any class > - final fields in a hidden class > > The spec of Field::set is changed to specify that records are not > modifiable via reflection. > Noe that no change in Field::setAccessible(true), i.e. it will succeed > to allow existing frameworks to have read access to final fields in > records. Just no write access. > > VarHandle does not support write access if it's a static final field or > an instance final field. > > This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and > `sun.misc.Unsafe::staticField{Offset/Base}` not to support records. > > No change is made in JNI. JNI could be considered to disallow > modification of final fields in records and hidden classes (static and > instance fields). But JNI has superpower and the current spec already > allows to modify the value of a final static field even after it's > constant folded (via JNI Set<type>Field and SetStatic<type>Field), then > all bets are off. This should be re-visited when we consider "final is > truly final" for all classes. > > Make final fields in records not modifiable via reflection enables JIT > optimization as these final fields are effectively truly final. > > This change impacts 3rd-party frameworks including 3rd-party > serialization framework that rely on core reflection `setAccessible` or > `sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to > construct records but not using the canonical constructor. > These frameworks would need to be updated to construct records via its > canonical constructor as done by the Java serialization. > > I see this change gives a good opportunity to engage the maintainers of > the serialization frameworks and work together to support new features > including records, inline classes and the new serialization mechanism > and which I think it is worth the investment. > > This is a low risk enhancement. I'd like to request approval for a late > enhancement in JDK 15. It extends the pre-existing code path with > refactoring the hidden classes to prepare for new kinds of classes with > trusted final fields. The change is straight-forward. > > Can this wait to integrate in JDK 16? > > It's important to get this enhancement in when record is a preview > feature that we can get feedback and give time to 3rd-party > serialization frameworks to look into adding the support for records. > If we delayed this change in 16 and records exit preview, it would be > bad for frameworks if they verify that they support records in 15 but > fail in 16. OTOH the risk of this patch is low. > > Performance Impact > > I addressed the performance concern I raised earlier myself. For > reflection, VM creates the reflective Field objects and fills in > MemberName when resolving a member. VM will tag if this > Field/MemberName is trusted final field. I think this is a cleaner > approach rather than in each place to check for final static and final > fields in hidden or record class to determine if it has write access or > not. > > `sun.misc.Unsafe` does not use Field::isTrustedFinalField because (1) > Unsafe has been allowing access of static final fields of any classes > but isTrustedFinalField is not limited to instance fields (2) Unsafe > disallows access to all fields in a hidden class (not limited to trusted > final fields). So it follows the precedence and simply checks if the > declaring class is a record. `Class::isRecord` calls > `Class::getSuperclass` to check if it's a subtype of `Record`. As > `Class::getSuperclass` is intrinsified, the call on isRecord on a normal > class is fast. Christoph has contributed the microbenchmarks that > confirm that no performance regression. > > Thanks > Mandy > [1] > https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040096.html
