RFR: 8352869: Verify.checkEQ: extension for NaN, VectorAPI and arbitrary Objects [v7]
Christian Hagedorn
chagedorn at openjdk.org
Wed Apr 2 15:02:13 UTC 2025
On Wed, 2 Apr 2025 12:17:31 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> We should extend the functionality of Verify.checkEQ:
>> - Allow different NaN encodings to be seen as equal (by default).
>> - Compare VectorAPI vectors.
>> - Compare Exceptions, and their messages.
>> - Compare arbitrary Objects via Reflection.
>>
>> Note: this is a prerequisite for the Template Library [JDK-8352861](https://bugs.openjdk.org/browse/JDK-8352861) / https://github.com/openjdk/jdk/pull/23418.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>
> refactor with checkEQWithRawBits
Thanks for the update! It's much easier to use and understand now I think.
I did a complete pass and left a lot of comments but mostly minor things. Overall, I think this looks great! :-)
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 32:
> 30: import java.lang.reflect.InvocationTargetException;
> 31: import java.util.HashMap;
> 32: import java.util.ArrayList;
Seems unused and can be removed.
Suggestion:
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 60:
> 58: private final boolean isFloatCheckWithRawBits;
> 59: private final HashMap<Object, Object> a2b = new HashMap<>();
> 60: private final HashMap<Object, Object> b2a = new HashMap<>();
Can you add a comment here what `a2b` and `b2a` means? See also some other comment further down about `a2b/b2a`, maybe you can share some docs or cross reference.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 67:
> 65:
> 66: /**
> 67: * Verify the content of two Objects, possibly recursively.
Maybe add:
Suggestion:
* Verify the contents of two Objects on a raw bit level, possibly recursively.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 68:
> 66: /**
> 67: * Verify the content of two Objects, possibly recursively.
> 68: * Different NaN encodins are considered non-qual, since we compare
Suggestion:
* Different NaN encodings are considered non-equal, since we compare
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 81:
> 79:
> 80: /**
> 81: * Verify the content of two Objects, possibly recursively.
Suggestion:
* Verify the contents of two Objects, possibly recursively.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 109:
> 107: Class ca = a.getClass();
> 108: Class cb = b.getClass();
> 109: if (ca != cb) {
Only seen this in my IDE: `ca` and `cb` should be `Class<?>` instead of the raw `Class` since `getClass()` returns a `Class<?>` (cannot make a suggestion since it's hidden here).
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 124:
> 122: switch (a) {
> 123: case Object[] x -> checkEQimpl(x, (Object[])b, field, aParent, bParent);
> 124: case Byte x -> checkEQimpl(x, ((Byte)b).byteValue(), field, aParent, bParent);
Can't you just pass `(Byte) b` to rely on auto unboxing instead?
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 143:
> 141: case Exception x -> checkEQimpl(x, (Exception) b, field, aParent, bParent);
> 142: default -> {
> 143: if (ca.getName().startsWith("jdk.incubator.vector") && ca.getName().contains("Vector")) {
Might be worth to extract this case to own methods and structure it like this to reduce the size of the method:
if (vectorClass()) {
checkEQForVectorAPIClass();
} else {
checkEQdispatch();
}
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 160:
> 158: } catch (InvocationTargetException e) {
> 159: throw new RuntimeException("Could not invoke toArray on " + ca.getName(), e);
> 160: }
You can merge them:
Suggestion:
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
throw new RuntimeException("Could not invoke toArray on " + ca.getName(), e);
}
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 187:
> 185: private void checkEQimpl(char a, char b, String field, Object aParent, Object bParent) {
> 186: if (a != b) {
> 187: System.err.println("ERROR: Verify.checkEQ failed: value mismatch: " + (int)a + " vs " + (int)b);
Why do you need an upcast here? Same for `short`.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 233:
> 231: * of an add or mul (NaN1 * NaN2 does not have same bits as NaN2 * NaN1, because the multiplication
> 232: * of two NaN values should always return the first of the two).
> 233: * Hence, by default, we pick the non-raw coparison: we verify that we have the same bit
Suggestion:
* Hence, by default, we pick the non-raw comparison: we verify that we have the same bit
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 236:
> 234: * pattern in all cases, except for NaN we project to the canonical NaN, using Float.floatToIntBits.
> 235: */
> 236: private boolean isFloatEQ(float a, float b) {
Shouldn't this be named `isFloatNotEQ` since you return true when they are different? Same for `isDoubleEQ` below. Alternatively: Return true when they are equal (i.e. flip condition).
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 242:
> 240:
> 241: /**
> 242: * See comments for "isFloatEQ".
We don't have Javadocs for the private methods but it could still help when navigating in the IDE to directly jump the the method when clicking on it (suggested same below for other places):
Suggestion:
* See comments for {@link #isFloatEQ}.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 250:
> 248:
> 249: /**
> 250: * Check that two floats are equal according to "isFloatEQ".
Suggestion:
* Check that two floats are equal according to {@link #isFloatEQ}.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 254:
> 252: private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) {
> 253: if (isFloatEQ(a, b)) {
> 254: System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);
Just noticed this now (there are other places as well): Since we now have `Verify.checkEQ()` and `Verify.checkEQWithRawBits()`, it would improve the readability if we reported which method was used. It could be done with something like that (pseudo code):
System.err.println("ERROR: Verify.checkEQ" + withRawBitsString() + " failed: value mismatch.
String withRawBitsString() {
return isFloatCheckWithRawBits ? "WithRawBits" : "";
}
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 256:
> 254: System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);
> 255: System.err.println(" Values: " + a + " vs " + b);
> 256: System.err.println(" Raw: " + Float.floatToRawIntBits(a) + " vs " + Float.floatToRawIntBits(b));
Do we always want to dump the raw bits even when `isFloatCheckWithRawBits` is false? I guess it does not hurt.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 263:
> 261:
> 262: /**
> 263: * Check that two doubles are equal according to "isDoubleEQ".
Suggestion:
* Check that two doubles are equal according to {@link #isDoubleEQ}.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 287:
> 285:
> 286: /**
> 287: * Verify that the content of two MemorySegments is identical. Note: we do not check the
Suggestion:
* Verify that the contents of two MemorySegments are identical. Note: we do not check the
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 316:
> 314: * Verify that the content of two MemorySegments is identical. Note: we do not check the
> 315: * backing type, only the size and content.
> 316: */
Probably a copy-paste error. Should be updated for exceptions.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 333:
> 331:
> 332: /**
> 333: * Verify that the content of two byte arrays is identical.
Suggestion:
* Verify that the contents of two byte arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 340:
> 338:
> 339: /**
> 340: * Verify that the content of two char arrays is identical.
Suggestion:
* Verify that the contents of two char arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 347:
> 345:
> 346: /**
> 347: * Verify that the content of two short arrays is identical.
Suggestion:
* Verify that the contents of two short arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 354:
> 352:
> 353: /**
> 354: * Verify that the content of two int arrays is identical.
Suggestion:
* Verify that the contents of two int arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 361:
> 359:
> 360: /**
> 361: * Verify that the content of two long arrays is identical.
Suggestion:
* Verify that the contents of two long arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 368:
> 366:
> 367: /**
> 368: * Check that two float arrays are equal according to "isFloatEQ".
Suggestion:
* Check that two float arrays are equal according to {@link #isFloatEQ}.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 387:
> 385:
> 386: /**
> 387: * Check that two double arrays are equal according to "isDoubleEQ".
Suggestion:
* Check that two double arrays are equal according to {@link #isDoubleEQ}.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 406:
> 404:
> 405: /**
> 406: * Verify that the content of two boolean arrays is identical.
Suggestion:
* Verify that the contents of two boolean arrays are identical.
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 425:
> 423:
> 424: /**
> 425: * Verify that the content of two Object arrays is identical, recursively:
Suggestion:
* Verify that the contents of two Object arrays are identical, recursively:
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 443:
> 441:
> 442: private void checkEQArbitraryClasses(Object a, Object b) {
> 443: Class c = a.getClass();
Suggestion:
Class<?> c = a.getClass();
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 447:
> 445: for (Field field : c.getDeclaredFields()) {
> 446: Object va = null;
> 447: Object vb = null;
`null` can be omitted:
Suggestion:
Object va;
Object vb;
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 463:
> 461: private void print(Object a, Object b, String field, Object aParent, Object bParent) {
> 462: System.err.println(" aParent: " + aParent);
> 463: System.err.println(" bParent: " + bParent);
Should we print `null` parents or just skip them?
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 481:
> 479: case Long x -> { return false; }
> 480: case Float x -> { return false; }
> 481: case Double x -> { return false; }
I think the convention is to us `_` when they are ignored. You can then also merge them:
Suggestion:
case Boolean _,
Byte _,
Short _,
Character _,
Integer _,
Long _,
Float _,
Double _ -> { return false; }
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 488:
> 486: Object aPrevious = b2a.get(b);
> 487: if (aPrevious == null && bPrevious == null) {
> 488: // Record for next time.
Can you explain, maybe as comment at `checkAlreadyVisited()`, why we want to have these caches?
test/hotspot/jtreg/compiler/lib/verify/Verify.java line 520:
> 518: long start = Long.max(offset - range, 0);
> 519: long end = Long.min(offset + range, a.byteSize());
> 520: for (long i = start; i < end; i++) {
Nit below: You can replace `System.err.println("")` with `System.err.println()`.
test/hotspot/jtreg/testlibrary_tests/verify/examples/TestWithVectorAPI.java line 2:
> 1: /*
> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
Suggestion:
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24224#pullrequestreview-2736390646
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024896216
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024897698
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024901854
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024898499
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024906090
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024917798
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024911572
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024921119
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024921953
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024928403
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024933737
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024970368
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024938566
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024939173
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024948324
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024953601
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024939637
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024954430
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024960438
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024961195
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024961541
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024961800
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024962110
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024962373
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024964374
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024964654
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024971548
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024984751
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024985775
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024988001
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024991410
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024995992
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2024999554
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2025002053
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2025002585
More information about the hotspot-compiler-dev
mailing list