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