-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8352869: Verify.checkEQ: extension for NaN, VectorAPI and arbitrary Objects #24224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8352869: Verify.checkEQ: extension for NaN, VectorAPI and arbitrary Objects #24224
Conversation
|
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
|
@eme64 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 14 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| */ | ||
| public static void checkEQ(Object a, Object b) { | ||
| checkEQ(a, b, ""); | ||
| public static void checkEQ(Object a, Object b, boolean isFloatCheckWithRawBits, boolean isCheckWithArbitraryClasses) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion. One boolean might be ok, but once you start adding 2 booleans it seems like a bit of a code smell to me. Do you envision more options being added? I would personally create a VerifyOptions record with the boolean flags options and pass that in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll experiment with a VerifyOptions, thanks for the suggestion!
| */ | ||
| private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) { | ||
| if (isFloatEQ(a, b)) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using a text block here make it more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@galderz What do you mean by a text block? String Templates would be nice, but we don't have them. Do you mean I should use String.format? But there is always so tricky to know what is going to be formatted to where...
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch for " + context); | ||
| private void checkEQimpl(double a, double b, String field, Object aParent, Object bParent) { | ||
| if (isDoubleEQ(a, b)) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same text block comment here
| } | ||
|
|
||
| private void print(Object a, Object b, String field, Object aParent, Object bParent) { | ||
| System.err.println(" aParent: " + aParent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text block?
| } catch (VerifyException e) {} | ||
| } | ||
|
|
||
| public static void checkNE(Object a, Object b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If checkEQ lives in Verify, wouldn't it make sense to also have checkNE there? Seems like the natural place making the API symmetric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I don't see the need for it being symmetric, i.e. for the API to have a checkNE. This checkNE method is just a convenience function to check that an exception is thrown if I expect it to.
|
@galderz Thanks for the comments! I think I addressed / answered all of them. Could you please have another look? :) |
| */ | ||
| private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) { | ||
| if (isFloatEQ(a, b)) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + verifyOptions.isFloatCheckWithRawBits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant using JEP 378 text blocks, e.g.
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + verifyOptions.isFloatCheckWithRawBits); | |
| System.err.printf(""" | |
| ERROR: Verify.checkEQ failed: value mismatch. check raw: %b | |
| Values: %.1f vs %.1f | |
| Raw: %d vs %d | |
| """, isFloatCheckWithRawBits, a, b, Float.floatToRawIntBits(a), Float.floatToRawIntBits(b)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
That has advantages and disadvantages.
Advantage: You can more easily see the "skeleton" of the test.
Disadvantage: Mapping the "holes" and the "values" is annoying, you basically have to count through each position. Plus it may end up being more lines.
Best would really be String Templates.
I asked @chhagedorn , he does not have an opinion either way.
Personally, I prefer my way, where you can easily see what values go where directly. But this is probably a taste question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice extensions! Some initial comments.
| */ | ||
|
|
||
| package compiler.lib.verify; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should update the copyright year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
| if (verifyOptions.isCheckWithArbitraryClasses) { | ||
| checkEQArbitraryClasses(a, b); | ||
| return; | ||
| } else { | ||
| System.err.println("ERROR: Verify.checkEQ failed: type not supported: " + ca.getName()); | ||
| print(a, b, field, aParent, bParent); | ||
| throw new VerifyException("Object type not supported: " + ca.getName() + " -- did you mean to 'enableCheckWithArbitraryClasses'?"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason behind throwing instead of just comparing two arbitrary objects by default? If a user calls Verify.checkEQ() and sees this exception, I would guess he then just passes the additional option and we have the same result. But maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think my reasoning was that comparing arbitrary classes requires reflection. And that is rather slow. So by default it would be good if that feature is not enabled, so the user tries to avoid it, and is aware when they enable it explicitly.
But if you think that is not useful, I can remove the feature.
@chhagedorn what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intention to let the user double check is good. I'm not sure though if the user is really aware of the potential slow down without diving deeper into the implementation. All they know is that checkEQ somehow does not support their some objects but there is a simple workaround to still use it. So, the real question is: How many users will then consider doing something different when facing this exception and not just enable it anyway? I guess enabling is probably the most natural thing to do.
Given that, I would probably just drop this. It would also simplify the API usage in the following way:
We would only have checks with NaNs being all equals and comparing raw bits (i.e. NaNs not equal). Then you could offer checkEQ() (default) and checkRawBitsEQ() or something like that. Then users do not need to worry about creating and passing in an Options.
What do you think about these suggestions?
What we could do either way at the checkEQ() API method: Describe the potential slow down with reflection when not using certain classes.
Co-authored-by: Christian Hagedorn <[email protected]>
|
@chhagedorn Thanks for the suggestions and questions! I think I addressed them all :) |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a closer look at the code later again :-)
| if (verifyOptions.isCheckWithArbitraryClasses) { | ||
| checkEQArbitraryClasses(a, b); | ||
| return; | ||
| } else { | ||
| System.err.println("ERROR: Verify.checkEQ failed: type not supported: " + ca.getName()); | ||
| print(a, b, field, aParent, bParent); | ||
| throw new VerifyException("Object type not supported: " + ca.getName() + " -- did you mean to 'enableCheckWithArbitraryClasses'?"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intention to let the user double check is good. I'm not sure though if the user is really aware of the potential slow down without diving deeper into the implementation. All they know is that checkEQ somehow does not support their some objects but there is a simple workaround to still use it. So, the real question is: How many users will then consider doing something different when facing this exception and not just enable it anyway? I guess enabling is probably the most natural thing to do.
Given that, I would probably just drop this. It would also simplify the API usage in the following way:
We would only have checks with NaNs being all equals and comparing raw bits (i.e. NaNs not equal). Then you could offer checkEQ() (default) and checkRawBitsEQ() or something like that. Then users do not need to worry about creating and passing in an Options.
What do you think about these suggestions?
What we could do either way at the checkEQ() API method: Describe the potential slow down with reflection when not using certain classes.
|
@chhagedorn Ok, I refactored it. I'm now always comparing arbitrary classes. And |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! :-)
| private final HashMap<Object, Object> a2b = new HashMap<>(); | ||
| private final HashMap<Object, Object> b2a = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some documentation :)
|
|
||
| private void print(Object a, Object b, String field, Object aParent, Object bParent) { | ||
| System.err.println(" aParent: " + aParent); | ||
| System.err.println(" bParent: " + bParent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we print null parents or just skip them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does not hurt to print null here. It makes the code a little simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, maybe we can print <none> in case of a null for more clarity?
| Object bPrevious = a2b.get(a); | ||
| Object aPrevious = b2a.get(b); | ||
| if (aPrevious == null && bPrevious == null) { | ||
| // Record for next time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain, maybe as comment at checkAlreadyVisited(), why we want to have these caches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation :)
| private void printMemorySegmentValue(MemorySegment a, long offset, int range) { | ||
| long start = Long.max(offset - range, 0); | ||
| long end = Long.min(offset + range, a.byteSize()); | ||
| for (long i = start; i < end; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit below: You can replace System.err.println("") with System.err.println().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
test/hotspot/jtreg/testlibrary_tests/verify/examples/TestWithVectorAPI.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Hagedorn <[email protected]>
|
@chhagedorn Thanks for the thorough review :) |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all my comments and doing the updates! I have some more final comments but then I think it's good to go from my side!
| private void checkEQimpl(char a, char b, String field, Object aParent, Object bParent) { | ||
| if (a != b) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch: " + (int)a + " vs " + (int)b + " for " + context); | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch: " + (int)a + " vs " + (int)b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense for the char case. But good that we could remove it for the short case.
| */ | ||
| private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) { | ||
| if (isFloatEQ(a, b)) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it could indeed be a little bit more complicated when you are deep down in a recursion. My thought was that it could be misleading when a test is using a mix of verifyEQ() and verifyEQWithRawBits() and you only read verifyEQ failed. You could be start looking at the wrong check even though the stack trace would have guided you to the correct place. Maybe we can just update "Verify.checkEQ" into something more generic like "Equality matching failed" and we're good. What do you think?
| if (isFloatEQ(a, b)) { | ||
| System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits); | ||
| System.err.println(" Values: " + a + " vs " + b); | ||
| System.err.println(" Raw: " + Float.floatToRawIntBits(a) + " vs " + Float.floatToRawIntBits(b)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, let's leave it in then!
| /** | ||
| * We do not want to import jdk.incubator.vector explicitly, because it would mean we would also have | ||
| * to add "--add-modules=jdk.incubator.vector" to the command-line of every test that uses the Verify | ||
| * class. So we hack this via reflection. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this background is only needed at checkEQForVectorAPIClass() (where you already have that comment). Here you can just describe what the code actually does or just drop the comment entirely since the method name is self-explanatory :-)
|
|
||
| private void print(Object a, Object b, String field, Object aParent, Object bParent) { | ||
| System.err.println(" aParent: " + aParent); | ||
| System.err.println(" bParent: " + bParent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, maybe we can print <none> in case of a null for more clarity?
Co-authored-by: Christian Hagedorn <[email protected]>
|
@chhagedorn Thanks for having another look! I applied all your suggestions :) |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me, thanks for bearing with me!
|
@galderz do you intend to review / approve this, or should I ask someone else? |
test/hotspot/jtreg/testlibrary_tests/verify/tests/TestVerify.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrey Turbanov <[email protected]>
|
@turbanoff Thanks for the two whitespace fixes :) |
|
@eme64 No, I don't think my review counts that much on this one. I think you need someone to review it that is has more background on the history of this. |
|
@galderz Christian already reviewed it, and generally it is ok for someone with deeper knowledge and someone with a little less familiarity to review. So far, on But totally up to you, I'm thankful for the comments you already left, and they do not obligate to do a complete review :) |
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I just found a few minor typos.
Co-authored-by: Tobias Hartmann <[email protected]>
|
@chhagedorn @TobiHartmann Thanks for the reviews :) /integrate |
|
@eme64 This pull request has not yet been marked as ready for integration. |
|
@chhagedorn Thanks! /integrate |
|
Going to push as commit 9f8fbf2.
Your commit was automatically rebased without conflicts. |
We should extend the functionality of Verify.checkEQ:
Note: this is a prerequisite for the Template Library JDK-8352861 / #23418.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24224/head:pull/24224$ git checkout pull/24224Update a local copy of the PR:
$ git checkout pull/24224$ git pull https://git.openjdk.org/jdk.git pull/24224/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24224View PR using the GUI difftool:
$ git pr show -t 24224Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24224.diff
Using Webrev
Link to Webrev Comment