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
