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

[Mandy Chung]

Thanks Coleen for the review.

Mandy

On 6/16/20 10:32 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 6/16/20 12:42 PM, Mandy Chung wrote:
>>
>>
>> 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.
>
> Thank you for this.  As far as I can see the trusted final field 
> doesn't add any footprint to the java.lang.reflect.Field instance.
>
> The VM runtime changes look okay to me, excluding the methodHandles.* 
> changes which I don't know that much about.
>
> Coleen
>
>> 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 
>>>>
>>
>