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

[Peter Levart]

Hi Aleksey,

On 6/18/20 7:10 PM, Aleksey Shipilev wrote:

On 6/18/20 3:09 PM, Chris Hegarty wrote:

On 18 Jun 2020, at 13:43, Aleksey Shipilev  wrote:
Again, JDK-8247532 is the writing on the wall: we don't need 3rd party 
developers to tell if Record
serialization works fast in 15 -- we already know it does not.

I disagree. JDK-8247532 is under review and well on its way towards JDK 15 
(yes, during RDP 1).
My reading of Peter’s benchmark result is that Record deserialization *does 
work fast*.   What am
I missing.

JDK-8247532 is the evidence that Records serialization performance story is not 
clear.

Even if we disregard that after JDK-8247532 [1] Records are still 8% slower, 
the _existence_ of
JDK-8247532 indicates there are performance problems in the area. That evidence 
now needs to be
compensated by much more evidence to the contrary. (Yes, I contracted a lot of 
Bayesian thinking
from my statistician wife...)

(Here were several paragraphs of further thoughts, but I realized it basically 
repeats what I said
before.)


Let me just express my thoughts on the subject of preventing writes to 
final fields in records in connection to deserialization. In short, I 
don't think any serialization framework would need to have direct write 
access to record fields in order to be performant. Serialization 
frameworks need write access to final fields in classical classes mainly 
because fields are generally encapsulated. If classes don't implement 
special API (constructors, methods, annotations ...) to interface with 
serialization framework (like for example Jackson annotations, etc.) 
then serialization framework can only interface such classes on the 
level of fields.


Now records are very constrained classes. They have an always present 
API that can be used to interface with serialization frameworks. For 
each field, they poses a constructor parameter with the same name and 
type and are arranged so that the fields are set from the constructor 
parameters. So instead of allocating a "zero" instance and setting 
fields, one can just invoke a canonical constructor which does the same. 
Such invocation is equally performant as allocating instance and setting 
fields from outside the constructor.


What I did in JDK-8247532 was that I just optimized the code that 
transforms the bytes read from stream into values for arguments of the 
canonical constructor using method handle transformations. That part was 
slow and not the fact that records are deserialized using their 
canonical constructor. Every serialization framework that works on the 
level of fields has to do the transformation of stream encoded data to 
field values - how it does that is what differs it from the rest - the 
final mechanism how those values are stored into object fields is not 
what makes it unique or performant.


There is also a desire that record constructor(s) are the only way to 
create instances of records such that any validation logic in the 
constructor(s) can't get bypassed. Not even via deserialization of 
"forged" streams. That way we can have records that are safer without 
scarifying performance.


And BTW, according to latest JDK-8247532 webrev.06, Java deserialization 
of records is not slower but sometimes even faster than deserialization 
of equivalent classical classes.



Regards, Peter

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

[Mandy Chung]

On 6/18/20 5:43 AM, Aleksey Shipilev wrote:

On 6/17/20 6:50 PM, Mandy Chung wrote:

On 6/17/20 8:11 AM, Aleksey Shipilev wrote:

On 6/15/20 11:58 PM, Mandy Chung wrote:
Amusingly, with the rest of Unsafe available the barn is still full open. 
Here's the sketch for the
workaround: get the field list with getDeclaredFields, optionally guess the VM 
layout (the rules are
not that hard, and JOL does it routinely), instantiate a Record with whatever 
arguments, poke the
test patterns with Unsafe.put*, read them back from record components to 
verify. It would get me the
same objectFieldOffset in a round-about way.

I'm aware of these workarounds in the wild.   As sun.misc.Unsafe is JDK 
unsupported API, this patch
does not attempt to implement a complete solution but adds an obvious 
notification informing the
frameworks that `sun.misc.Unsafe::objectFieldOffset` and 
`sun.misc.Unsafe::staticField{Offset/Base}`
not to support records.

I understand the intent. My point is that intent is mistimed during this time. 
I understand this
tripwire needs to be put while Records are still in preview. My point is that 
it cannot be put in
before Records serialization story has the preview-proven answer.

The intent also looks rather opaque. The intent is to notify maintainers, fine. 
I am one of the
maintainers (although not of serialization library, but still heavy-Unsafe 
one), so hear me out. I
came up with the workaround above in about 15 minutes after someone pointed out 
the exception to me.
Have it crossed my mind that maybe JDK developers want some help here? No. It 
looks like another
impediment that should be solved on the spot. Does that exception communicate 
any intent at all? The
message is generic. There is no comment. How would you expect to push 
maintainers to think along the
"we should be collaborating to find a proper way to do this", instead of "this 
is set in stone; let
me hack this around too"?


Records is in second preview (JEP 384).  As clearly stated in JEP 12 and 
[1],  introducing a feature as a preview feature enables developers to 
provide feedbacks before it becoems permanent in a future Java SE Platform.


We should encourage developers to engage and provide feedbacks and have 
the discussion during preview.


For JOL, have you raised an enhancement request to the serviceability 
team? Inspecting on an object layout would be useful and I suggest you 
file a RFE if you have done so.


[1] 
https://docs.oracle.com/en/java/javase/14/language/preview-language-and-vm-features.html

This is the actionable bit:

At very least the exception message should say something about the intent. And 
maybe even the
comment in Unsafe.java should point to the discussion about this intent and 
maybe even provide the
breadcrumbs to follow, e.g. to ObjectInputStream.readRecord().


To hit this exception, the app must be running with --enable-preview.   
There are documentation about preview feature and warning messages 
etc.   To avoid duplicating the info, the best I could do may be to add 
"preview feature" in the exception message?


Chris Hegarty will definitely want to help out w.r.t. the serialization 
frameworks in supporting records.


Mandy

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

[Aleksey Shipilev]

On 6/18/20 3:09 PM, Chris Hegarty wrote:
>> On 18 Jun 2020, at 13:43, Aleksey Shipilev  wrote:
>> Again, JDK-8247532 is the writing on the wall: we don't need 3rd party 
>> developers to tell if Record
>> serialization works fast in 15 -- we already know it does not.
> 
> I disagree. JDK-8247532 is under review and well on its way towards JDK 15 
> (yes, during RDP 1).
> My reading of Peter’s benchmark result is that Record deserialization *does 
> work fast*.   What am
> I missing.
JDK-8247532 is the evidence that Records serialization performance story is not 
clear.

Even if we disregard that after JDK-8247532 [1] Records are still 8% slower, 
the _existence_ of
JDK-8247532 indicates there are performance problems in the area. That evidence 
now needs to be
compensated by much more evidence to the contrary. (Yes, I contracted a lot of 
Bayesian thinking
from my statistician wife...)

(Here were several paragraphs of further thoughts, but I realized it basically 
repeats what I said
before.)

-- 
Thanks,
-Aleksey

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067223.html
 RecordSerializationBench.deserializeClasses:  31.049 ± 0.235   us/op
 RecordSerializationBench.deserializeRecords:  33.588 ± 0.394   us/op

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

[Chris Hegarty]

> On 18 Jun 2020, at 13:43, Aleksey Shipilev  wrote:
> 
>> ...
> 
> Again, JDK-8247532 is the writing on the wall: we don't need 3rd party 
> developers to tell if Record
> serialization works fast in 15 -- we already know it does not.

I disagree. JDK-8247532 is under review and well on its way towards JDK 15 
(yes, during RDP 1). My reading of Peter’s benchmark result is that Record 
deserialization *does work fast*.   What am I missing.

-Chris.

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

[Aleksey Shipilev]

On 6/17/20 6:50 PM, Mandy Chung wrote:
> On 6/17/20 8:11 AM, Aleksey Shipilev wrote:
>> On 6/15/20 11:58 PM, Mandy Chung wrote:
>> Amusingly, with the rest of Unsafe available the barn is still full open. 
>> Here's the sketch for the
>> workaround: get the field list with getDeclaredFields, optionally guess the 
>> VM layout (the rules are
>> not that hard, and JOL does it routinely), instantiate a Record with 
>> whatever arguments, poke the
>> test patterns with Unsafe.put*, read them back from record components to 
>> verify. It would get me the
>> same objectFieldOffset in a round-about way.
> 
> I'm aware of these workarounds in the wild.   As sun.misc.Unsafe is JDK 
> unsupported API, this patch
> does not attempt to implement a complete solution but adds an obvious 
> notification informing the
> frameworks that `sun.misc.Unsafe::objectFieldOffset` and 
> `sun.misc.Unsafe::staticField{Offset/Base}`
> not to support records.

I understand the intent. My point is that intent is mistimed during this time. 
I understand this
tripwire needs to be put while Records are still in preview. My point is that 
it cannot be put in
before Records serialization story has the preview-proven answer.

The intent also looks rather opaque. The intent is to notify maintainers, fine. 
I am one of the
maintainers (although not of serialization library, but still heavy-Unsafe 
one), so hear me out. I
came up with the workaround above in about 15 minutes after someone pointed out 
the exception to me.
Have it crossed my mind that maybe JDK developers want some help here? No. It 
looks like another
impediment that should be solved on the spot. Does that exception communicate 
any intent at all? The
message is generic. There is no comment. How would you expect to push 
maintainers to think along the
"we should be collaborating to find a proper way to do this", instead of "this 
is set in stone; let
me hack this around too"?

This is the actionable bit:

At very least the exception message should say something about the intent. And 
maybe even the
comment in Unsafe.java should point to the discussion about this intent and 
maybe even provide the
breadcrumbs to follow, e.g. to ObjectInputStream.readRecord().

>> Are we absolutely sure that what ObjectInputStream.readRecord() does fits 
>> the 3rd party
>> serialization libraries? Does it work for them performance-wise? 
> 
> There should be performance improvement opportunity (see Peter Levart's good 
> work - JDK-8247532 [1])
> 
>> (Are we even sure about that for JDK itself?)
> 
> Java serialization support for records use its canonical constructor.  (I'm 
> not sure what you tried
> to point out by this).

Let me try again.

The existence of JDK-8247532 solidifies my concern: the JDK code itself had not 
yet figured out how
to do Record serialization fast! Now put yourself in the shoes of the 3rd party 
lib maintainer:
Unsafe is forbidden, JDK code itself is slow. Asking 3rd party lib maintainer 
to find whatever
intricate incantation they should use to get decent performance -- while JDK 
itself is still working
on it! -- puts them into weird position, from where the *sane* way out is to 
hack around the Unsafe
restriction. I mean, it is as enticing as hacking the JDK code itself, with the 
added benefit of
working across the JDK releases.

This is time and again the core of Unsafe discussion, which boils down to a 
very simple request: if
JDK developers take away some Unsafe APIs, 3rd party developers need to have 
the known good
replacement for it. If that does not happen, 3rd party developers would invest 
in doing more Unsafe
hacks.

Record serialization seems to fall into the same trap. To reiterate: the 
non-Unsafe code should be
proven as the viable alternative before breaking Unsafe. Otherwise, we only 
deepen the dependency on
Unsafe.


>>  Because if it is not, 3rd party lib maintainers would proceed to hacking in 
>> the
>> "objectFieldOffset workaround" and we would get the cobra-effect-like 
>> strengthening of dependency on
>> Unsafe quirks.
> 
> This is exactly why we request this for 15 so that 3rd-party lib maintainers 
> can prototype and send
> feedback.  We love to understand any stumbling block and work together to 
> resolve any issue before
> records exit preview.

Again, JDK-8247532 is the writing on the wall: we don't need 3rd party 
developers to tell if Record
serialization works fast in 15 -- we already know it does not.

>> It is fun to consider JNI to be more powerful than Unsafe. This seems 
>> backwards. The intent to break
>> Unsafe users might be defensible, but this power oddity is still quite odd.
> 
> I think you misread the message.  There is no claim whether JNI or Unsafe is 
> more powerful. 

I don't think I did. Disallowing Unsafe to do something that JNI is allowed to 
do is making Unsafe
less powerful. This was nothing to do with what API is supported or not.

-- 
Thanks,
-Aleksey

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

[Mandy Chung]

On 6/17/20 7:34 PM, serguei.spit...@oracle.com wrote:

Hi Mandy,

This looks good from the Serviceability point of view.

> 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 SetField and SetStaticField), then all bets are 
off.
> This should be re-visited when we consider "final is truly final" 
for all classes.


This can potentially impact the JDWP agent as it uses the JNI to set 
fields values.

Please, see ObjectReference#setValue:
https://docs.oracle.com/en/java/javase/14/docs/api/jdk.jdi/com/sun/jdi/ObjectReference.html#setValue(com.sun.jdi.Field,com.sun.jdi.Value) 





Thanks for pointing out this API.   I will file a RFE to follow this up.

Mandy


Thanks,
Serguei


On 6/15/20 14: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 SetField and 
SetStaticField), 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.

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

[serguei.spit...@oracle.com]

Hi Mandy,

This looks good from the Serviceability point of view.

> 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 SetField and SetStaticField), then all bets are off.
> This should be re-visited when we consider "final is truly final" for 
all classes.


This can potentially impact the JDWP agent as it uses the JNI to set 
fields values.

Please, see ObjectReference#setValue:
https://docs.oracle.com/en/java/javase/14/docs/api/jdk.jdi/com/sun/jdi/ObjectReference.html#setValue(com.sun.jdi.Field,com.sun.jdi.Value)

Thanks,
Serguei


On 6/15/20 14: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 SetField and SetStaticField), 
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

Re: (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 SetField and 
SetStaticField), 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 

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

[Mandy Chung]

On 6/17/20 8:11 AM, Aleksey Shipilev wrote:

On 6/15/20 11:58 PM, Mandy Chung wrote:

This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.

Note this would break otherwise innocuous introspection for records, for 
example dumping the VM
layout with JOL:
   http://openjdk.java.net/projects/code-tools/jol/


To me, JOL needs a supported API in the serviceability area to 
introspect a field layout.  JOL will not work for inline types without 
adding new APIs.


sun.misc.Unsafe is an unsupported API.   My proposal for sun.misc.Unsafe 
(in fact jdk.unsupported module) is to keep the support for legacy use 
but not necessarily for new features.




Amusingly, with the rest of Unsafe available the barn is still full open. 
Here's the sketch for the
workaround: get the field list with getDeclaredFields, optionally guess the VM 
layout (the rules are
not that hard, and JOL does it routinely), instantiate a Record with whatever 
arguments, poke the
test patterns with Unsafe.put*, read them back from record components to 
verify. It would get me the
same objectFieldOffset in a round-about way.


I'm aware of these workarounds in the wild.   As sun.misc.Unsafe is JDK 
unsupported API, this patch does not attempt to implement a complete 
solution but adds an obvious notification informing the frameworks that 
`sun.misc.Unsafe::objectFieldOffset` and 
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.


Unsafe is unsafe!   Use it at your own risk.


And speaking from JOL maintainer standpoint, that one would be very tempting to 
do, because it would
not depend on whatever protection shenanigans a particular JDK release tries to 
enforce.


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.

Are we absolutely sure that what ObjectInputStream.readRecord() does fits the 
3rd party
serialization libraries? Does it work for them performance-wise?


There should be performance improvement opportunity (see Peter Levart's 
good work - JDK-8247532 [1])



(Are we even sure about that for JDK itself?)


Java serialization support for records use its canonical constructor.  
(I'm not sure what you tried to point out by this).



  Because if it is not, 3rd party lib maintainers would proceed to hacking in 
the
"objectFieldOffset workaround" and we would get the cobra-effect-like 
strengthening of dependency on
Unsafe quirks.


This is exactly why we request this for 15 so that 3rd-party lib 
maintainers can prototype and send feedback.  We love to understand any 
stumbling block and work together to resolve any issue before records 
exit preview.





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 SetField and SetStaticField), then
all bets are off.

It is fun to consider JNI to be more powerful than Unsafe. This seems 
backwards. The intent to break
Unsafe users might be defensible, but this power oddity is still quite odd.



I think you misread the message.  There is no claim whether JNI or 
Unsafe is more powerful.


JNI is a supported API.   The above explains why I propose no spec 
change to make in JNI.  OTOH jdk.unsupported is unsupported but has more 
love by frameworks to avoid writing in native.  Adding a check in some 
sun.misc.Unsafe APIs (even it can be hacked to workaround it) is a clear 
notification what it does not intend to support.


Hope this helps.
Mandy
[1] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067223.html

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

[Aleksey Shipilev]

On 6/15/20 11:58 PM, Mandy Chung wrote:
> This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and 
> `sun.misc.Unsafe::staticField{Offset/Base}` not to support records.

Note this would break otherwise innocuous introspection for records, for 
example dumping the VM
layout with JOL:
  http://openjdk.java.net/projects/code-tools/jol/

Amusingly, with the rest of Unsafe available the barn is still full open. 
Here's the sketch for the
workaround: get the field list with getDeclaredFields, optionally guess the VM 
layout (the rules are
not that hard, and JOL does it routinely), instantiate a Record with whatever 
arguments, poke the
test patterns with Unsafe.put*, read them back from record components to 
verify. It would get me the
same objectFieldOffset in a round-about way.

And speaking from JOL maintainer standpoint, that one would be very tempting to 
do, because it would
not depend on whatever protection shenanigans a particular JDK release tries to 
enforce.

> 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.

Are we absolutely sure that what ObjectInputStream.readRecord() does fits the 
3rd party
serialization libraries? Does it work for them performance-wise? (Are we even 
sure about that for
JDK itself?) Because if it is not, 3rd party lib maintainers would proceed to 
hacking in the
"objectFieldOffset workaround" and we would get the cobra-effect-like 
strengthening of dependency on
Unsafe quirks.

> 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 SetField and SetStaticField), then 
> all bets are off.

It is fun to consider JNI to be more powerful than Unsafe. This seems 
backwards. The intent to break
Unsafe users might be defensible, but this power oddity is still quite odd.

-- 
Thanks,
-Aleksey

Re: (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 SetField and SetStaticField), 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 

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

[John Rose]

On Jun 15, 2020, at 2:58 PM, Mandy Chung  wrote:
> 
> Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/ 
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8247517 
> 

Good work.  Reviewed.  — John

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

[Mandy Chung]

Hi David,

Thanks for the review.

On 6/15/20 7:29 PM, David Holmes wrote:

Hi Mandy, Christoph,

The code changes here all look okay to me. The idea of introducing the 
notion of "trusted final fields" seems quite reasonable.


I made one editorial comment on the CSR request.



Thanks.
I'm unclear if the new TEST.properties file needs a copyright notice 
and header. We have 706 such files in the repo and 554 (mostly 
hotspot) do have the copyright notice and header.




I will find out the recommendation and update you.

Mandy


Thanks,
David

On 16/06/2020 7:58 am, 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 SetField and 
SetStaticField), 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 

Re: (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" 
À: "hotspot-dev" , "core-libs-dev" 
, "amber-dev"

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 SetField and SetStaticField), 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

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

[Remi Forax]

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" 
> À: "hotspot-dev" , "core-libs-dev" 
> , "amber-dev"
> 
> 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 SetField and SetStaticField), 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

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

[David Holmes]

Hi Mandy, Christoph,

The code changes here all look okay to me. The idea of introducing the 
notion of "trusted final fields" seems quite reasonable.


I made one editorial comment on the CSR request.

I'm unclear if the new TEST.properties file needs a copyright notice and 
header. We have 706 such files in the repo and 554 (mostly hotspot) do 
have the copyright notice and header.


Thanks,
David

On 16/06/2020 7:58 am, 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 SetField and SetStaticField), 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 

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

[Mandy Chung]

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 SetField and SetStaticField), 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