Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v6]
On Fri, 22 Dec 2023 13:00:11 GMT, Sergey Tsypanov wrote:
>> Currently if we create a record it's fields are compared in their
>> declaration order. This might be ineffective in cases when two objects have
>> "heavy" fields equals to each other, but different "lightweight" fields
>> (heavy and lightweight in terms of comparison) e.g. primitives, enums,
>> nullable/non-nulls etc.
>>
>> If we have declared a record like
>>
>> public record MyRecord(String field1, int field2) {}
>>
>> It's equals() looks like:
>>
>> public final equals(Ljava/lang/Object;)Z
>>L0
>> LINENUMBER 3 L0
>> ALOAD 0
>> ALOAD 1
>> INVOKEDYNAMIC
>> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
>> [
>> // handle kind 0x6 : INVOKESTATIC
>>
>> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
>> // arguments:
>> com.caspianone.openbanking.productservice.controller.MyRecord.class,
>> "field1;field2",
>> // handle kind 0x1 : GETFIELD
>>
>> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
>> // handle kind 0x1 : GETFIELD
>> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
>> ]
>> IRETURN
>>L1
>> LOCALVARIABLE this
>> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
>> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
>> MAXSTACK = 2
>> MAXLOCALS = 2
>>
>> This can be improved by rearranging the comparison order of the fields
>> moving enums and primitives upper, and collections/arrays lower.
>
> Sergey Tsypanov has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Merge remote-tracking branch 'origin/record-equals' into record-equals
> - 8322292: Add test case
Anything else I can do about it?
-
PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1902184686
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v5]
On Thu, 21 Dec 2023 19:43:50 GMT, ExE Boss wrote:
>> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 224:
>>
>>> 222: var rt2 = mh2.type().returnType();
>>> 223: return Integer.compare(
>>> 224: rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ? 1
>>> : Iterable.class.isAssignableFrom(rt1) ? -1 : 0,
>>
>> Doesn't this put primitives, enums and arrays at the end instead of at the
>> start? I've tried this with a simple array:
>>
>> Class[] types = { int.class, String.class, List.class, long.class,
>> TimeUnit.class, byte[].class, Integer.class };
>>
>> The result of sorting:
>>
>> Class[7] { interface java.util.List, class java.lang.String, class
>> java.lang.Integer, int, long, class java.util.concurrent.TimeUnit, class [B }
>>
>> By switching the -1 and 1 I get the primitives etc. at the start:
>>
>> Class[7] { int, long, class java.util.concurrent.TimeUnit, class [B, class
>> java.lang.String, class java.lang.Integer, interface java.util.List }
>> ``
>
> The `equalator`s are joined together in reverse order, so this is actually
> correct for the current implementation:
>
>
> record Example(A a, B b, C c) {
> public static void main(String... args) {
> final var left = new Example(new A(), new B(), new C());
> final var right = new Example(new A(), new B(), new C());
>
> left.equals(right);
> // prints:
> // > C::equals()
> // > B::equals()
> // > A::equals()
> }
> }
>
> record A() {
> @Override
> public boolean equals(Object other) {
> System.out.println("A::equals()");
> return other instanceof A;
> }
> }
>
> record B() {
> @Override
> public boolean equals(Object other) {
> System.out.println("B::equals()");
> return other instanceof B;
> }
> }
>
> record C() {
> @Override
> public boolean equals(Object other) {
> System.out.println("C::equals()");
> return other instanceof C;
> }
> }
I've added the test case for the current algorithm
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1435034161
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v6]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with two additional
commits since the last revision:
- Merge remote-tracking branch 'origin/record-equals' into record-equals
- 8322292: Add test case
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/158d9a83..037a3fd1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=05
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=04-05
Stats: 42 lines in 1 file changed: 41 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v5]
On Thu, 21 Dec 2023 18:30:40 GMT, Rob Spoor wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/lang/runtime/ObjectMethods.java
>>
>> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 224:
>
>> 222: var rt2 = mh2.type().returnType();
>> 223: return Integer.compare(
>> 224: rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ? 1
>> : Iterable.class.isAssignableFrom(rt1) ? -1 : 0,
>
> Doesn't this put primitives, enums and arrays at the end instead of at the
> start? I've tried this with a simple array:
>
> Class[] types = { int.class, String.class, List.class, long.class,
> TimeUnit.class, byte[].class, Integer.class };
>
> The result of sorting:
>
> Class[7] { interface java.util.List, class java.lang.String, class
> java.lang.Integer, int, long, class java.util.concurrent.TimeUnit, class [B }
>
> By switching the -1 and 1 I get the primitives etc. at the start:
>
> Class[7] { int, long, class java.util.concurrent.TimeUnit, class [B, class
> java.lang.String, class java.lang.Integer, interface java.util.List }
> ``
The `equalator`s are joined together in reverse order, so this is actually
correct for the current implementation:
record Example(A a, B b, C c) {
public static void main(String... args) {
final var left = new Example(new A(), new B(), new C());
final var right = new Example(new A(), new B(), new C());
left.equals(right);
// prints:
// > C::equals()
// > B::equals()
// > A::equals()
}
}
record A() {
@Override
public boolean equals(Object other) {
System.out.println("A::equals()");
return other instanceof A;
}
}
record B() {
@Override
public boolean equals(Object other) {
System.out.println("B::equals()");
return other instanceof B;
}
}
record C() {
@Override
public boolean equals(Object other) {
System.out.println("C::equals()");
return other instanceof C;
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1434439484
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v5]
On Thu, 21 Dec 2023 16:58:00 GMT, Sergey Tsypanov wrote:
>> Currently if we create a record it's fields are compared in their
>> declaration order. This might be ineffective in cases when two objects have
>> "heavy" fields equals to each other, but different "lightweight" fields
>> (heavy and lightweight in terms of comparison) e.g. primitives, enums,
>> nullable/non-nulls etc.
>>
>> If we have declared a record like
>>
>> public record MyRecord(String field1, int field2) {}
>>
>> It's equals() looks like:
>>
>> public final equals(Ljava/lang/Object;)Z
>>L0
>> LINENUMBER 3 L0
>> ALOAD 0
>> ALOAD 1
>> INVOKEDYNAMIC
>> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
>> [
>> // handle kind 0x6 : INVOKESTATIC
>>
>> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
>> // arguments:
>> com.caspianone.openbanking.productservice.controller.MyRecord.class,
>> "field1;field2",
>> // handle kind 0x1 : GETFIELD
>>
>> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
>> // handle kind 0x1 : GETFIELD
>> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
>> ]
>> IRETURN
>>L1
>> LOCALVARIABLE this
>> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
>> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
>> MAXSTACK = 2
>> MAXLOCALS = 2
>>
>> This can be improved by rearranging the comparison order of the fields
>> moving enums and primitives upper, and collections/arrays lower.
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/runtime/ObjectMethods.java
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 224:
> 222: var rt2 = mh2.type().returnType();
> 223: return Integer.compare(
> 224: rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ? 1 :
> Iterable.class.isAssignableFrom(rt1) ? -1 : 0,
Doesn't this put primitives, enums and arrays at the end instead of at the
start? I've tried this with a simple array:
Class[] types = { int.class, String.class, List.class, long.class,
TimeUnit.class, byte[].class, Integer.class };
The result of sorting:
Class[7] { interface java.util.List, class java.lang.String, class
java.lang.Integer, int, long, class java.util.concurrent.TimeUnit, class [B }
By switching the -1 and 1 I get the primitives etc. at the start:
Class[7] { int, long, class java.util.concurrent.TimeUnit, class [B, class
java.lang.String, class java.lang.Integer, interface java.util.List }
``
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1434386929
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v5]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with one additional
commit since the last revision:
Update src/java.base/share/classes/java/lang/runtime/ObjectMethods.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/9c8ae4fb..158d9a83
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=04
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=03-04
Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v4]
On Thu, 21 Dec 2023 14:47:43 GMT, ExE Boss wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8322292: Tiny improvement
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 227:
>
>> 225: }
>> 226: return rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ?
>> 1 : Iterable.class.isAssignableFrom(rt1) ? -1 : 0;
>> 227: });
>
> The way this comparator is currently implemented, the `signum(c.compare(mh1,
> mh2)) == -signum(c.compare(mh2, mh1))` assertion won’t hold, which will cause
> the sorting to be unstable.
This should ensure stable ordering:
Suggestion:
Arrays.sort(equalsGetters, (mh1, mh2) -> {
var rt1 = mh1.type().returnType();
var rt2 = mh2.type().returnType();
return Integer.compare(
rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ? 1 :
Iterable.class.isAssignableFrom(rt1) ? -1 : 0,
rt2.isPrimitive() || rt2.isEnum() || rt2.isArray() ? 1 :
Iterable.class.isAssignableFrom(rt2) ? -1 : 0
);
});
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1434173126
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v4]
On Wed, 20 Dec 2023 10:11:58 GMT, Sergey Tsypanov wrote:
>> Currently if we create a record it's fields are compared in their
>> declaration order. This might be ineffective in cases when two objects have
>> "heavy" fields equals to each other, but different "lightweight" fields
>> (heavy and lightweight in terms of comparison) e.g. primitives, enums,
>> nullable/non-nulls etc.
>>
>> If we have declared a record like
>>
>> public record MyRecord(String field1, int field2) {}
>>
>> It's equals() looks like:
>>
>> public final equals(Ljava/lang/Object;)Z
>>L0
>> LINENUMBER 3 L0
>> ALOAD 0
>> ALOAD 1
>> INVOKEDYNAMIC
>> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
>> [
>> // handle kind 0x6 : INVOKESTATIC
>>
>> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
>> // arguments:
>> com.caspianone.openbanking.productservice.controller.MyRecord.class,
>> "field1;field2",
>> // handle kind 0x1 : GETFIELD
>>
>> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
>> // handle kind 0x1 : GETFIELD
>> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
>> ]
>> IRETURN
>>L1
>> LOCALVARIABLE this
>> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
>> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
>> MAXSTACK = 2
>> MAXLOCALS = 2
>>
>> This can be improved by rearranging the comparison order of the fields
>> moving enums and primitives upper, and collections/arrays lower.
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8322292: Tiny improvement
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 227:
> 225: }
> 226: return rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ?
> 1 : Iterable.class.isAssignableFrom(rt1) ? -1 : 0;
> 227: });
The way this comparator is currently implemented, the `signum(c.compare(mh1,
mh2)) == -signum(c.compare(mh2, mh1))` assertion won’t hold, which will cause
the sorting to be unstable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1434166820
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v4]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with one additional
commit since the last revision:
8322292: Tiny improvement
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/dded977f..9c8ae4fb
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=03
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=02-03
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()
On Tue, 19 Dec 2023 11:01:12 GMT, Hannes Greule wrote: > Isn't Arrays.equals() used under the hood? No, for arrays == is used - PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1863374656
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v3]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with one additional
commit since the last revision:
8322292: Shift arrays up
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/988601b3..dded977f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=02
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()
On Tue, 19 Dec 2023 09:41:45 GMT, Sergey Tsypanov wrote: > Isn't `Arrays.equals()` used under the hood? The JLS and the API spec don't mention any special-casing of arrays, and the code seems to use `Objects.equals` for all non-primitive types: https://github.com/openjdk/jdk/blob/988601b324c971daf4e537074a007c25059af91b/src/java.base/share/classes/java/lang/runtime/ObjectMethods.java#L183-L187 (side note, it seems like `OBJECT_EQUALS` is unused?) - PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1862550269
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v2]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with one additional
commit since the last revision:
8322292: Use copy
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/26c4428c..988601b3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=01
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=00-01
Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()
On Tue, 19 Dec 2023 06:07:31 GMT, Hannes Greule wrote: > Arrays are compared by reference Isn't `Arrays.equals()` used under the hood? > You are sorting the array passed to the bootstrap method Good point, fixed. - PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1862429275
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()
On Mon, 18 Dec 2023 13:42:35 GMT, Sergey Tsypanov wrote:
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Generally, this is a good idea. A few comments:
1. Arrays are compared by reference rather than element-wise, so they
should/could be at the beginning similar to enums, not at the end.
2. You are sorting the array passed to the bootstrap method. This might be
observable to callers, potentially causing problems, especially when e.g.
creating the method handle for equals first, and then for toString with the
same array.
3. What's the test situation here? Is there enough coverage for things like
that already?
-
PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1862179369
RFR: 8322292: Rearrange comparison of fields in Record.equals()
Currently if we create a record it's fields are compared in their declaration
order. This might be ineffective in cases when two objects have "heavy" fields
equals to each other, but different "lightweight" fields (heavy and lightweight
in terms of comparison) e.g. primitives, enums, nullable/non-nulls etc.
If we have declared a record like
public record MyRecord(String field1, int field2) {}
It's equals() looks like:
public final equals(Ljava/lang/Object;)Z
L0
LINENUMBER 3 L0
ALOAD 0
ALOAD 1
INVOKEDYNAMIC
equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
[
// handle kind 0x6 : INVOKESTATIC
java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
// arguments:
com.caspianone.openbanking.productservice.controller.MyRecord.class,
"field1;field2",
// handle kind 0x1 : GETFIELD
com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
// handle kind 0x1 : GETFIELD
com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
]
IRETURN
L1
LOCALVARIABLE this
Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
MAXSTACK = 2
MAXLOCALS = 2
This can be improved by rearranging the comparison order of the fields moving
enums and primitives upper, and collections/arrays lower.
-
Commit messages:
- 8322292: Update copyright
- 8322292: Shift arrays and Iterables down
- Improve Record.equals()
Changes: https://git.openjdk.org/jdk/pull/17143/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17143=00
Issue: https://bugs.openjdk.org/browse/JDK-8322292
Stats: 12 lines in 1 file changed: 9 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
