Trust final fields in records
Christoph Dreis
christoph.dreis at freenet.de
Fri Jun 12 06:32:39 UTC 2020
Hi Mandy,
Thanks for taking this. Highly appreciated.
I have attached a patch with the needed changes – I think – on the reflection side of things with an additional test.
But yeah, I would need help completing the picture in case I’m missing anything.
Cheers,
Christoph
Von: Mandy Chung <mandy.chung at oracle.com>
Datum: Freitag, 12. Juni 2020 um 03:57
An: Brian Goetz <brian.goetz at oracle.com>, Christoph Dreis <christoph.dreis at freenet.de>
Cc: Amber dev <amber-dev at openjdk.java.net>, valhalla-dev <valhalla-dev at openjdk.java.net>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>
Betreff: Re: Trust final fields in records
Hi Christoph,
I can sponsor your patch. I create https://bugs.openjdk.java.net/browse/JDK-8247444.
Do you want to contribute to the core reflection change? I can help too.
Mandy
On 6/11/20 3:23 PM, Brian Goetz wrote:
Yes, please.
On 6/11/2020 5:49 PM, Mandy Chung wrote:
I really like to see "final fields truly final" at least start with the new features such as inline classes and records.
Final fields of hidden classes have no write access [1] regardless of the accessible flag. I'd propose to make final fields of records and inline classes non-modifiable in a similar fashion as hidden classes.
Mandy
[1] https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/lang/reflect/Field.html#set(java.lang.Object,java.lang.Object)
On 6/11/20 1:38 PM, Christoph Dreis wrote:
Hi,
I’ve played around with records the other day and wondered if their (final) fields could be maybe trusted.
This would allow further optimizations to kick in.
E.g. with the following benchmark:
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Benchmark)
public class MyBenchmark {
static final Rectangle rectangle;
static {
rectangle = new Rectangle(1, 1);
}
record Rectangle(int length, int width) {
public int size() {
return length * width;
}
}
@Benchmark public int testSize() { return 1000 / rectangle.size(); }
}
I see the following results when I apply the attached patch:
Benchmark Mode Cnt Score Error Units
MyBenchmark.testSizeBefore avgt 10 3,873 ± 0,044 ns/op
MyBenchmark.testSizePatched avgt 10 1,740 ± 0,058 ns/op
After all, records state that they are "shallowly immutable" - whatever " shallowly" means here.
The risk that I see here is that people could still use reflection on records to change fields - for reasons.
Maybe that aspect could be tightened though before records go non-experimental in favor of the optimization?
I wonder if this could be considered. If so, I would highly appreciate it if someone can sponsor the patch.
Let me know what you think.
Cheers,
Christoph
===== PATCH =====
diff -r 984fde9a0b7f src/hotspot/share/ci/ciField.cpp
--- a/src/hotspot/share/ci/ciField.cpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciField.cpp Thu Jun 11 22:25:02 2020 +0200
@@ -231,6 +231,9 @@
// Trust final fields in all boxed classes
if (holder->is_box_klass())
return true;
+ // Trust final fields in records
+ if (holder->is_record())
+ return true;
// Trust final fields in String
if (holder->name() == ciSymbol::java_lang_String())
return true;
diff -r 984fde9a0b7f src/hotspot/share/ci/ciInstanceKlass.cpp
--- a/src/hotspot/share/ci/ciInstanceKlass.cpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp Thu Jun 11 22:25:02 2020 +0200
@@ -64,6 +64,7 @@
_has_nonstatic_concrete_methods = ik->has_nonstatic_concrete_methods();
_is_unsafe_anonymous = ik->is_unsafe_anonymous();
_is_hidden = ik->is_hidden();
+ _is_record = ik->is_record();
_nonstatic_fields = NULL; // initialized lazily by compute_nonstatic_fields:
_has_injected_fields = -1;
_implementor = NULL; // we will fill these lazily
@@ -125,6 +126,7 @@
_has_injected_fields = -1;
_is_unsafe_anonymous = false;
_is_hidden = false;
+ _is_record = false;
_loader = loader;
_protection_domain = protection_domain;
_is_shared = false;
diff -r 984fde9a0b7f src/hotspot/share/ci/ciInstanceKlass.hpp
--- a/src/hotspot/share/ci/ciInstanceKlass.hpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciInstanceKlass.hpp Thu Jun 11 22:25:02 2020 +0200
@@ -57,6 +57,7 @@
bool _has_nonstatic_concrete_methods;
bool _is_unsafe_anonymous;
bool _is_hidden;
+ bool _is_record;
ciFlags _flags;
jint _nonstatic_field_size;
@@ -200,6 +201,10 @@
return _is_hidden;
}
+ bool is_record() const {
+ return _is_record;
+ }
+
ciInstanceKlass* get_canonical_holder(int offset);
ciField* get_field_by_offset(int field_offset, bool is_static);
ciField* get_field_by_name(ciSymbol* name, ciSymbol* signature, bool is_static);
-------------- next part --------------
diff -r 984fde9a0b7f src/hotspot/share/ci/ciField.cpp
--- a/src/hotspot/share/ci/ciField.cpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciField.cpp Fri Jun 12 08:12:44 2020 +0200
@@ -231,6 +231,9 @@
// Trust final fields in all boxed classes
if (holder->is_box_klass())
return true;
+ // Trust final fields in records
+ if (holder->is_record())
+ return true;
// Trust final fields in String
if (holder->name() == ciSymbol::java_lang_String())
return true;
diff -r 984fde9a0b7f src/hotspot/share/ci/ciInstanceKlass.cpp
--- a/src/hotspot/share/ci/ciInstanceKlass.cpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciInstanceKlass.cpp Fri Jun 12 08:12:44 2020 +0200
@@ -64,6 +64,7 @@
_has_nonstatic_concrete_methods = ik->has_nonstatic_concrete_methods();
_is_unsafe_anonymous = ik->is_unsafe_anonymous();
_is_hidden = ik->is_hidden();
+ _is_record = ik->is_record();
_nonstatic_fields = NULL; // initialized lazily by compute_nonstatic_fields:
_has_injected_fields = -1;
_implementor = NULL; // we will fill these lazily
@@ -125,6 +126,7 @@
_has_injected_fields = -1;
_is_unsafe_anonymous = false;
_is_hidden = false;
+ _is_record = false;
_loader = loader;
_protection_domain = protection_domain;
_is_shared = false;
diff -r 984fde9a0b7f src/hotspot/share/ci/ciInstanceKlass.hpp
--- a/src/hotspot/share/ci/ciInstanceKlass.hpp Tue Jun 09 16:22:54 2020 +0000
+++ b/src/hotspot/share/ci/ciInstanceKlass.hpp Fri Jun 12 08:12:44 2020 +0200
@@ -57,6 +57,7 @@
bool _has_nonstatic_concrete_methods;
bool _is_unsafe_anonymous;
bool _is_hidden;
+ bool _is_record;
ciFlags _flags;
jint _nonstatic_field_size;
@@ -200,6 +201,10 @@
return _is_hidden;
}
+ bool is_record() const {
+ return _is_record;
+ }
+
ciInstanceKlass* get_canonical_holder(int offset);
ciField* get_field_by_offset(int field_offset, bool is_static);
ciField* get_field_by_name(ciSymbol* name, ciSymbol* signature, bool is_static);
diff -r 984fde9a0b7f src/java.base/share/classes/java/lang/invoke/MethodHandles.java
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Tue Jun 09 16:22:54 2020 +0000
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java Fri Jun 12 08:12:44 2020 +0200
@@ -3270,6 +3270,7 @@
return unreflectField(f, true);
}
+ @SuppressWarnings("preview")
private MethodHandle unreflectField(Field f, boolean isSetter) throws IllegalAccessException {
MemberName field = new MemberName(f, isSetter);
if (isSetter && field.isFinal()) {
@@ -3277,6 +3278,8 @@
throw field.makeAccessException("static final field has no write access", this);
} else if (field.getDeclaringClass().isHidden()){
throw field.makeAccessException("final field in a hidden class has no write access", this);
+ } else if (field.getDeclaringClass().isRecord()){
+ throw field.makeAccessException("final field in a record class has no write access", this);
}
}
assert(isSetter
@@ -3806,6 +3809,7 @@
final boolean checkSecurity = false;
return getFieldVarHandleCommon(getRefKind, putRefKind, refc, getField, putField, checkSecurity);
}
+ @SuppressWarnings("preview")
private VarHandle getFieldVarHandleCommon(byte getRefKind, byte putRefKind,
Class<?> refc, MemberName getField, MemberName putField,
boolean checkSecurity) throws IllegalAccessException {
@@ -3839,7 +3843,9 @@
refc = lookupClass();
}
return VarHandles.makeFieldHandle(getField, refc, getField.getFieldType(),
- this.allowedModes == TRUSTED && !getField.getDeclaringClass().isHidden());
+ this.allowedModes == TRUSTED
+ && !getField.getDeclaringClass().isHidden()
+ && !getField.getDeclaringClass().isRecord());
}
/** Check access and get the requested constructor. */
private MethodHandle getDirectConstructor(Class<?> refc, MemberName ctor) throws IllegalAccessException {
diff -r 984fde9a0b7f src/java.base/share/classes/java/lang/reflect/AccessibleObject.java
--- a/src/java.base/share/classes/java/lang/reflect/AccessibleObject.java Tue Jun 09 16:22:54 2020 +0000
+++ b/src/java.base/share/classes/java/lang/reflect/AccessibleObject.java Fri Jun 12 08:12:44 2020 +0200
@@ -177,10 +177,11 @@
* to the caller's module. </p>
*
* <p> This method cannot be used to enable {@linkplain Field#set <em>write</em>}
- * access to a final field declared in a {@linkplain Class#isHidden() hidden class},
- * since such fields are not modifiable. The {@code accessible} flag when
- * {@code true} suppresses Java language access control checks to only
- * enable {@linkplain Field#get <em>read</em>} access to such fields.
+ * access to a final field declared in a {@linkplain Class#isHidden() hidden class}
+ * or a {@linkplain Class#isRecord() record} since such fields are not modifiable.
+ * The {@code accessible} flag when {@code true} suppresses Java language access
+ * control checks to only enable {@linkplain Field#get <em>read</em>} access to
+ * such fields.
*
* <p> If there is a security manager, its
* {@code checkPermission} method is first called with a
diff -r 984fde9a0b7f src/java.base/share/classes/java/lang/reflect/Field.java
--- a/src/java.base/share/classes/java/lang/reflect/Field.java Tue Jun 09 16:22:54 2020 +0000
+++ b/src/java.base/share/classes/java/lang/reflect/Field.java Fri Jun 12 08:12:44 2020 +0200
@@ -729,6 +729,8 @@
* <li>the field is non-static; and</li>
* <li>the field's declaring class is not a {@linkplain Class#isHidden()
* hidden class}.</li>
+ * <li>the field's declaring class is not a {@linkplain Class#isRecord()
+ * record class}.</li>
* </ul>
* If any of the above checks is not met, this method throws an
* {@code IllegalAccessException}.
diff -r 984fde9a0b7f src/java.base/share/classes/jdk/internal/reflect/UnsafeFieldAccessorFactory.java
--- a/src/java.base/share/classes/jdk/internal/reflect/UnsafeFieldAccessorFactory.java Tue Jun 09 16:22:54 2020 +0000
+++ b/src/java.base/share/classes/jdk/internal/reflect/UnsafeFieldAccessorFactory.java Fri Jun 12 08:12:44 2020 +0200
@@ -29,13 +29,14 @@
import java.lang.reflect.Modifier;
class UnsafeFieldAccessorFactory {
+ @SuppressWarnings("preview")
static FieldAccessor newFieldAccessor(Field field, boolean override) {
Class<?> type = field.getType();
boolean isStatic = Modifier.isStatic(field.getModifiers());
boolean isFinal = Modifier.isFinal(field.getModifiers());
boolean isVolatile = Modifier.isVolatile(field.getModifiers());
boolean isQualified = isFinal || isVolatile;
- boolean isReadOnly = isFinal && (isStatic || !override || field.getDeclaringClass().isHidden());
+ boolean isReadOnly = isFinal && (isStatic || !override || field.getDeclaringClass().isHidden() || field.getDeclaringClass().isRecord());
if (isStatic) {
// This code path does not guarantee that the field's
// declaring class has been initialized, but it must be
diff -r 984fde9a0b7f src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
--- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java Tue Jun 09 16:22:54 2020 +0000
+++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java Fri Jun 12 08:12:44 2020 +0200
@@ -636,13 +636,18 @@
* @see #getInt(Object, long)
*/
@ForceInline
+ @SuppressWarnings("preview")
public long objectFieldOffset(Field f) {
if (f == null) {
throw new NullPointerException();
}
- if (f.getDeclaringClass().isHidden()) {
+ Class<?> declaringClass = f.getDeclaringClass();
+ if (declaringClass.isHidden()) {
throw new UnsupportedOperationException("can't get field offset on a hidden class: " + f);
}
+ if (declaringClass.isRecord()) {
+ throw new UnsupportedOperationException("can't get field offset on a record class: " + f);
+ }
return theInternalUnsafe.objectFieldOffset(f);
}
@@ -664,13 +669,18 @@
* @see #getInt(Object, long)
*/
@ForceInline
+ @SuppressWarnings("preview")
public long staticFieldOffset(Field f) {
if (f == null) {
throw new NullPointerException();
}
- if (f.getDeclaringClass().isHidden()) {
+ Class<?> declaringClass = f.getDeclaringClass();
+ if (declaringClass.isHidden()) {
throw new UnsupportedOperationException("can't get field offset on a hidden class: " + f);
}
+ if (declaringClass.isRecord()) {
+ throw new UnsupportedOperationException("can't get field offset on a record class: " + f);
+ }
return theInternalUnsafe.staticFieldOffset(f);
}
@@ -685,13 +695,18 @@
* this class.
*/
@ForceInline
+ @SuppressWarnings("preview")
public Object staticFieldBase(Field f) {
if (f == null) {
throw new NullPointerException();
}
- if (f.getDeclaringClass().isHidden()) {
+ Class<?> declaringClass = f.getDeclaringClass();
+ if (declaringClass.isHidden()) {
throw new UnsupportedOperationException("can't get base address on a hidden class: " + f);
}
+ if (declaringClass.isRecord()) {
+ throw new UnsupportedOperationException("can't get base address on a record class: " + f);
+ }
return theInternalUnsafe.staticFieldBase(f);
}
diff -r 984fde9a0b7f test/jdk/java/lang/reflect/records/RecordReflectionTest.java
--- a/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Tue Jun 09 16:22:54 2020 +0000
+++ b/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Fri Jun 12 08:12:44 2020 +0200
@@ -187,4 +187,19 @@
assertEquals(f.getAnnotatedType().getAnnotations().length, 1);
assertEquals(f.getAnnotatedType().getAnnotations()[0].toString(), annos[0].toString());
}
+
+ public void testReadOnlyFieldInRecord() throws Throwable {
+ R2 o = new R2(1, 2);
+ Class<?> recordClass = R2.class;
+ String fieldName = "i";
+ Field f = recordClass.getDeclaredField(fieldName);
+ assertTrue(f.trySetAccessible());
+ assertTrue(f.get(o) != null);
+ try {
+ f.set(o, null);
+ assertTrue(false, "should fail to set " + fieldName);
+ } catch (IllegalAccessException e) {
+ }
+ }
+
}
More information about the valhalla-dev
mailing list