Trust final fields in records
Mandy Chung
mandy.chung at oracle.com
Fri Jun 12 20:24:36 UTC 2020
Hi Remi,
VarHandle does not support write access mode on final fields (see the
spec of Lookup::findVarHandle[1] and Lookup::unreflectVarHandle[2]). I
agree and we need new tests to verify it.
Christoph,
Can you please add the tests for VarHandle and sun.misc.Unsafe access to
records? You can reference these tests:
test/jdk/java/lang/invoke/defineHiddenClass/UnreflectTest.java
test/jdk/java/lang/reflect/AccessibleObject/HiddenClassTest.java
test/jdk/sun/misc/UnsafeFieldOffsets.java
Regarding the patch, I'm a little concerned on the performance impact to
check if it's a record as Class::isRecord is currently a JVM native call
in particular in UnsafeFieldAccessorFactory and sun.misc.Unsafe. I can
add a MemberName::isTrustedFinalField. I have to look into ways to
avoid the JVM call to check if a Field's declaring class is a record.
Mandy
[1]
https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findVarHandle(java.lang.Class,java.lang.String,java.lang.Class)
[2]
https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#unreflectVarHandle(java.lang.reflect.Field)
On 6/12/20 6:01 AM, Remi Forax wrote:
> Hi Christoph,
> do you have checked if VarHandle access is disable,
> because VarHandle are built on to of jdk.internal.misc.Unsafe and not sun.misc.Unafe ?
>
> otherwise it looks good but Unsafe tests are missing.
>
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "Christoph Dreis" <christoph.dreis at freenet.de>
>> À: "mandy chung" <mandy.chung at oracle.com>
>> Cc: "amber-dev" <amber-dev at openjdk.java.net>, "hotspot-runtime-dev" <hotspot-runtime-dev at openjdk.java.net>,
>> "valhalla-dev" <valhalla-dev at openjdk.java.net>
>> Envoyé: Vendredi 12 Juin 2020 08:32:39
>> Objet: Re: Trust final fields in records
>> 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);
More information about the valhalla-dev
mailing list