Trust final fields in records

[Remi Forax]

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