Trust final fields in records

[Christoph Dreis]

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 --------------
An embedded and charset-unspecified text was scrubbed...
Name: trust-record-fields.txt
URL: <https://mail.openjdk.java.net/pipermail/amber-dev/attachments/20200612/f8a38822/trust-record-fields-0001.txt>