Trust final fields in records

[Remi Forax]

----- Mail original -----
> De: "mandy chung" <mandy.chung 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"
> <hotspot-runtime-dev at openjdk.java.net>
> Envoyé: Jeudi 11 Juin 2020 23:49:29
> Objet: Re: Trust final fields in records

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

Hi Christoph, hi Mandy,
for records, when i propose the same idea, Brian replies that it will make harder to switch back and forth between a class and a record, that's why record final fields are are not truly final.

> 
> Mandy
> 

Rémi

> [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);
>>