(15) RFR: JDK-8247444: Trust final fields in records

[Vladimir Ivanov]

Looks good!

I welcome and fully support such hardening of final field handling for 
new language features.

Minor comment on naming: "trusted final field" is an JVM implementation 
detail and I'd prefer to keep it internal to JVM. (I hope it'll 
eventually go away and will be replaced with a more disciplined approach.)

Probably, a better alternative is a name that clearly communicates that 
the JVM expects/allows modifications of a final field. Or, at least, 
that it's *the JVM* which "trusts" the contents of a final field after 
its initialization is over (vmTrustedFinal/isVMTrustedFinalField).

But I'm fine with keeping the patch as is.

Best regards,
Vladimir Ivanov

On 16.06.2020 00:58, Mandy Chung wrote:
> 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 
>