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

[Mandy Chung]

Hi Vladimir,

Thanks for the review and feedback.

FWIW.  The spec of java.lang.reflect.AccessibleObject and Field 
specifies which final fields can be modified or not.  It use the 
*non-modifiable* term and JVM can rely on.

I will keep the patch as is.  I'm happy to clean this up when a more 
disciplined approach is coming.

Mandy

On 6/17/20 4:13 AM, Vladimir Ivanov wrote:
> 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 
>>