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

[Mandy Chung]

On 6/16/20 2:49 AM, Remi Forax wrote:
> 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.

Adding a new field instead of using the modifiers is a deliberate choice 
from my side.  It's higher risk to overload the modifiers to include 
VM-specific flags.   I am all for a better clean up (valhalla lworld 
does use the modifiers to cover a flag indicating if it's flattened 
which I am not satisfied either).

> So +1

Thanks Remi.

Mandy

> Rémi
>
> ----- Mail original -----
>> De: "mandy chung" <mandy.chung at oracle.com>
>> À: "hotspot-dev" <hotspot-dev at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>, "amber-dev"
>> <amber-dev at 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