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