Trust final fields in records

[Mandy Chung]

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