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