Trust final fields in records

Mandy Chung mandy.chung at oracle.com
Fri Jun 12 20:51:29 UTC 2020



On 6/12/20 1:42 PM, Christoph Dreis wrote:
>
> Hi Mandy,
>
> I will try to add tests for the missing pieces over the weekend.
>
> About your concern of calling Class::isRecord. Isn’t that already a 
> problem with Class::isHidden?
>
> After all, Class:isHidden is a native call as well, but is annotated 
> with @HotSpotIntrinsicCandidate.
>
> Is that maybe something which should be added to Class::isRecord as well?
>

Class::isHidden is intrinsified.  We could intrinsify Class::isRecord 
that is one of the options I considered.  This impacts Graal which I am 
finding out its implication if we want to target this for 15 (I think 
it's straight forward)

I'm trying to look for other options with less impact if any.

Mandy

> Cheers,
>
> Christoph
>
> *Von: *Mandy Chung <mandy.chung at oracle.com>
> *Datum: *Freitag, 12. Juni 2020 um 22:25
> *An: *Remi Forax <forax at univ-mlv.fr>, Christoph Dreis 
> <christoph.dreis at freenet.de>
> *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>
> *Betreff: *Re: Trust final fields in records
>
> 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>  <mailto:christoph.dreis at freenet.de>
>
>         À: "mandy chung"<mandy.chung at oracle.com>  <mailto:mandy.chung at oracle.com>
>
>         Cc: "amber-dev"<amber-dev at openjdk.java.net>  <mailto:amber-dev at openjdk.java.net>, "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>  <mailto:hotspot-runtime-dev at openjdk.java.net>,
>
>         "valhalla-dev"<valhalla-dev at openjdk.java.net>  <mailto: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>  <mailto:mandy.chung at oracle.com>
>
>         Datum: Freitag, 12. Juni 2020 um 03:57
>
>         An: Brian Goetz<brian.goetz at oracle.com>  <mailto:brian.goetz at oracle.com>, Christoph Dreis
>
>         <christoph.dreis at freenet.de>  <mailto:christoph.dreis at freenet.de>
>
>         Cc: Amber dev<amber-dev at openjdk.java.net>  <mailto:amber-dev at openjdk.java.net>, valhalla-dev
>
>         <valhalla-dev at openjdk.java.net>  <mailto:valhalla-dev at openjdk.java.net>,"hotspot-runtime-dev at openjdk.java.net"  <mailto:hotspot-runtime-dev at openjdk.java.net>
>
>         <hotspot-runtime-dev at openjdk.java.net>  <mailto: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