Skip to content
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

8229186: Improve error messages for TestStringIntrinsics failures #112

Closed
wants to merge 14 commits into from

Conversation

lepestock
Copy link
Contributor

@lepestock lepestock commented Sep 10, 2020

pre-Skara RFR thread: link

Error reporting was improved by writing a C-style escaped string representations for the variables passed to the methods being tested. For array comparisons, a dedicated diff-formatter was implemented.

Sample output for comparing byte arrays (with artificial failure):

 Result: (false) of 'arrayEqualsB' is not equal to expected (true)
 Arrays differ starting from [index: 7]:
 ... 5, 6,   7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, ...
 ... 5, 6, 125, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, ...
           ^^^^
 java.lang.RuntimeException: Result: (false) of 'arrayEqualsB' is not 
 equal to expected (true)
          at 
 compiler.intrinsics.string.TestStringIntrinsics.invokeAndCheckArrays(TestStringIntrinsics.java:273) 
           at ... stack trace continues - E.N.

Sample output for comparing char arrays:

 Result: (false) of 'arrayEqualsC' is not equal to expected (true)
 Arrays differ starting from [index: 7]:
 ... \\u0005, \\u0006, \\u0007, \\u0008, \\u0009, \\n, ...
 ... \\u0005, \\u0006,      }, \\u0008, \\u0009, \\n, ...
                     ^^^^^^^
 java.lang.RuntimeException: Result: (false) of 'arrayEqualsC' is not 
 equal to expected (true)
          at 
 compiler.intrinsics.string.TestStringIntrinsics.invokeAndCheckArrays(TestStringIntrinsics.java:280) 
           at
 ... and so on - E.N.

testing: open/test/hotspot/jtreg/compiler/intrinsics/string/TestStringIntrinsics.java on linux, windows, macosx.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8229186: Improve error messages for TestStringIntrinsics failures

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/112/head:pull/112
$ git checkout pull/112

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 10, 2020

👋 Welcome back enikitin! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 10, 2020
@openjdk
Copy link

openjdk bot commented Sep 10, 2020

@lepestock The following labels will be automatically applied to this pull request: core-libs hotspot-compiler.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@mlbridge
Copy link

mlbridge bot commented Sep 10, 2020

@lepestock
Copy link
Contributor Author

Responding to the comments from pre-Skara thread:

test/hotspot/jtreg/compiler/intrinsics/string/TestStringIntrinsics.java:
I'd prefer invokeAndCompareArrays and invokeAndCheck to be as close as possible: have both of them to accept either boolean or Object as 2nd arg; print/throw the same error message

the invokeAndCheck is very generic, it can be called with different objects and expect any kind of result, not only boolean. Therefore its output format radically differs from what an array-comparator should show.

maybe I'm missing smth, but I don't understand why ArrayCodec supports only char and byte arrays; and hence I don't understand why you need ArrayCodec::of methods, as you can simply do new ArrayCoded(Arrays.stream(a).collect(Collectors.toList()) where a is an array of any type

for Object arrays, one can use that.
for integer primitives one needs Arrays.stream(a).boxed.collect(Collectors.toList()), please note 'boxed' - it is required and not generic.
for bytes or chars, there is none (no overload methos in the Arrays.stream(a));
To sum up, I can't see how with the given type system and utilities set I can make in a better, less wordy way. I've added int and long overloads, support for String and Object arrays to make it more complete.

it seems that ArrayCodec should be an inner static class of ArrayDiff

I would argue that - I find it useful for printing arrays (and this usage has been utilised in the TestStringIntrinsics.java). In addition, I dont' like the practice of making such huge classes an inner classes as this reduces readability and modularity.

Other issues have been fixed. I added support for int, long, Object and String arrays.

@iignatev
Copy link
Member

Responding to the comments from pre-Skara thread:

test/hotspot/jtreg/compiler/intrinsics/string/TestStringIntrinsics.java:
I'd prefer invokeAndCompareArrays and invokeAndCheck to be as close as possible: have both of them to accept either boolean or Object as 2nd arg; print/throw the same error message

the invokeAndCheck is very generic, it can be called with different objects and expect any kind of result, not only boolean. Therefore its output format radically differs from what an array-comparator should show.

I am not sure I understand what you mean...

  1. granted you can't change invokeAndCheck's 2nd argument to bool as there are other values being passed, but you can change invokeAndCompareArrays to accept an Object and compare expected and actual values by Object::equals.
  2. even if you can't change output of these two methods to be the same (which I so far failed to see why), you still can change invokeAndCheck's message var to include actual and expected values in the same way as invokeAndCompareArrays does.

maybe I'm missing smth, but I don't understand why ArrayCodec supports only char and byte arrays; and hence I don't understand why you need ArrayCodec::of methods, as you can simply do new ArrayCoded(Arrays.stream(a).collect(Collectors.toList()) where a is an array of any type

for Object arrays, one can use that.
for integer primitives one needs Arrays.stream(a).boxed.collect(Collectors.toList()), please note 'boxed' - it is required and not generic.
for bytes or chars, there is none (no overload methos in the Arrays.stream(a));
To sum up, I can't see how with the given type system and utilities set I can make in a better, less wordy way. I've added int and long overloads, support for String and Object arrays to make it more complete.

you don't need ArrayCodec::of(Object array) anymore, do you?

it seems that ArrayCodec should be an inner static class of ArrayDiff

I would argue that - I find it useful for printing arrays (and this usage has been utilised in the TestStringIntrinsics.java). In addition, I dont' like the practice of making such huge classes an inner classes as this reduces readability and modularity.

oki.

Other issues have been fixed. I added support for int, long, Object and String arrays.

@lepestock
Copy link
Contributor Author

  1. granted you can't change invokeAndCheck's 2nd argument to bool as there are other values being passed, but you can change invokeAndCompareArrays to accept an Object and compare expected and actual values by Object::equals.

Well, I think that would make invokeAndCompareArrays look less specific and confuse a reader by creating a false impression that it could be called with an Object as an expected result. In reality, the method is only called with 'equals' of different signatures, and 'equals' does always return boolean. It could've been named 'invokeEqualsOnArraysAndCompareThem` :)

The other method, invokeAndCheck is different. It can call, for example, String.concat("abc", "def") and expect the String "abcdef" as result. It does really need to be generic.

  1. even if you can't change output of these two methods to be the same (which I so far failed to see why), you still can change invokeAndCheck's message var to include actual and expected values in the same way as invokeAndCompareArrays does.

invokeAndCompareArrays / ArrayDiff compares two huge arrays and present a nice short slice in the difference area. The short slice is possible because we do a limited task - compare arrays.
invokeAndCheck , on the other hand, can have as parameters the utf16, a string of 10K symbols. Do we really need a 10K string as output in our log?

you don't need ArrayCodec::of(Object array) anymore, do you?

Unfortunately, it is used in the ArrayCodec.format (which is used in TestStringIntrinsics.java) - to make it possible to call it with everything and not swamp the code with overloads.

@iignatev
Copy link
Member

The other method, invokeAndCheck is different. It can call, for example, String.concat("abc", "def") and expect the String "abcdef" as result. It does really need to be generic.
although I still don't see them as really being that much different, I'm fine w/ keeping them as-is.

invokeAndCompareArrays / ArrayDiff compares two huge arrays and present a nice short slice in the difference area. The short slice is possible because we do a limited task - compare arrays.
invokeAndCheck , on the other hand, can have as parameters the utf16, a string of 10K symbols. Do we really need a 10K string as output in our log?

I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having relative information in the exception is always a good thing. should 10K symbols become a problem (this though would also mean that you can't print the compared values w/ Format.asLiteral), we can revisit this.

you don't need ArrayCodec::of(Object array) anymore, do you?

Unfortunately, it is used in the ArrayCodec.format (which is used in TestStringIntrinsics.java) - to make it possible to call it with everything and not swamp the code with overloads.

hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in ArrayDiff::of, do you?

@lepestock
Copy link
Contributor Author

I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having relative information in the exception is always a good thing. should 10K symbols become a problem (this though would also mean that you can't print the compared values w/ Format.asLiteral), we can revisit this.

Ok, I got it. I was blind and stupid, sorry. Fixed, please check the e6fb6d0

hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in ArrayDiff::of, do you?

<sigh>... I can see no better solution here. The ArrayDiff::of checks that both component types are the same in addition, but I probably could have avoided duplicating the switch (we can ignore double guessing the type). The issue is that in ArrayDiff is forwarded to first and second, I would need to declare them as generic (ArrayCodec). The type system seem to not allow me to fix that nicely.

@iignatev
Copy link
Member

I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having relative information in the exception is always a good thing. should 10K symbols become a problem (this though would also mean that you can't print the compared values w/ Format.asLiteral), we can revisit this.

Ok, I got it. I was blind and stupid, sorry. Fixed, please check the e6fb6d0

I'd use StringBuilder to construct the message.

hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in ArrayDiff::of, do you?

... I can see no better solution here. The ArrayDiff::of checks that both component types are the same in addition, but I probably could have avoided duplicating the switch (we can ignore double guessing the type). The issue is that in ArrayDiff is forwarded to first and second, I would need to declare them as generic (ArrayCodec). The type system seem to not allow me to fix that nicely.

wouldn't the following patch do?

--- a/test/lib/jdk/test/lib/format/ArrayDiff.java
+++ b/test/lib/jdk/test/lib/format/ArrayDiff.java
@@ -123,41 +123,10 @@ public class ArrayDiff<E> implements Diff {
         if (!bothAreArrays || !componentTypesAreSame) {
             throw new IllegalArgumentException("Both arguments should be arrays of the same type");
         }
-
-        var type = first.getClass().getComponentType();
-        if (type == byte.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((byte[])first),
-                    ArrayCodec.of((byte[])second),
-                    width, contextBefore);
-        } else if (type == int.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((int[])first),
-                    ArrayCodec.of((int[])second),
-                    width, contextBefore);
-        } else if (type == long.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((long[])first),
-                    ArrayCodec.of((long[])second),
-                    width, contextBefore);
-        } else if (type == char.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((char[])first),
-                    ArrayCodec.of((char[])second),
-                    width, contextBefore);
-        } else if (type == String.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((String[])first),
-                    ArrayCodec.of((String[])second),
-                    width, contextBefore);
-        } else if (!type.isPrimitive() && !type.isArray()) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((Object[])first),
-                    ArrayCodec.of((Object[])second),
-                    width, contextBefore);
-        }
-
-        throw new IllegalArgumentException("Unsupported array component type: " + type);
+        return new ArrayDiff(
+                ArrayCodec.of(first),
+                ArrayCodec.of(second),
+                width, contextBefore);
     }
 
     /**

@lepestock
Copy link
Contributor Author

I'd use StringBuilder to construct the message.

Fixed, please check the f0b1df8.

wouldn't the following patch do?

Wow, that worked. That type erasure... pushed in the same commit f0b1df8.

Copy link
Member

@iignatev iignatev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* @library /test/lib
* @run testng jdk.test.lib.format.ArrayDiffTest
*/
public class ArrayDiffTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be it makes sense to check null values. Also, reversed checks. For example there is first = [1, ..], second = []. But no check for first = [], second = [1, ..] for some cases,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the 39992fc. That ended in finding a minor issue, actually (ArrayDiff.java:180). Sometimes reversing can be valuable, thanks!

@openjdk
Copy link

openjdk bot commented Oct 2, 2020

@lepestock 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:

8229186: Improve error messages for TestStringIntrinsics failures

Reviewed-by: iignatyev, lmesnik

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 455 new commits pushed to the master branch:

  • 6d2c1a6: 8254292: Update JMH devkit to 1.26
  • 2bbf8a2: 8245543: Cgroups: Incorrect detection logic on some systems (still reproducible)
  • aaa0a2a: 8254297: Zero and Minimal VMs are broken with undeclared identifier 'DerivedPointerTable' after JDK-8253180
  • 7e80c98: 8254261: fix javadocs in jdk.test.lib.Utils
  • d4b5dfd: 8253857: Shenandoah: Bugs in ShenandoahEvacOOMHandler related code
  • e9c1905: 8253740: [PPC64] Minor interpreter cleanup
  • b1448da: 8253900: SA: wrong size computation when JVM was built without AOT
  • 2bc8bc5: 8254265: s390 and linux 32 bit builds broken
  • 4f9a1ff: 8254073: Tokenizer improvements (revised)
  • 9cecc16: 8254244: Some code emitted by TemplateTable::branch is unused when running TieredCompilation
  • ... and 445 more: https://git.openjdk.java.net/jdk/compare/c8257ea4e254bd4f8c574f781a6f6d8c658a1bca...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@iignatev, @lmesnik) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 2, 2020
@lepestock
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 3, 2020
@openjdk
Copy link

openjdk bot commented Oct 3, 2020

@lepestock
Your change (at version 39992fc) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Member

shipilev commented Oct 8, 2020

@iignatev, @lmesnik: would you like to sponsor this?


}

return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interesting notice, actually. It should be either -1 or result depending on whether arrays are equal, or one array is a prefix of another. Fixed in the 174148a.

@openjdk openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Oct 8, 2020
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 9, 2020
@lepestock
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 9, 2020
@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@lepestock
Your change (at version 174148a) is now ready to be sponsored by a Committer.

@iignatev
Copy link
Member

iignatev commented Oct 9, 2020

/sponsor

@openjdk openjdk bot closed this Oct 9, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 9, 2020
@openjdk
Copy link

openjdk bot commented Oct 9, 2020

@iignatev @lepestock Since your change was applied there have been 455 commits pushed to the master branch:

  • 6d2c1a6: 8254292: Update JMH devkit to 1.26
  • 2bbf8a2: 8245543: Cgroups: Incorrect detection logic on some systems (still reproducible)
  • aaa0a2a: 8254297: Zero and Minimal VMs are broken with undeclared identifier 'DerivedPointerTable' after JDK-8253180
  • 7e80c98: 8254261: fix javadocs in jdk.test.lib.Utils
  • d4b5dfd: 8253857: Shenandoah: Bugs in ShenandoahEvacOOMHandler related code
  • e9c1905: 8253740: [PPC64] Minor interpreter cleanup
  • b1448da: 8253900: SA: wrong size computation when JVM was built without AOT
  • 2bc8bc5: 8254265: s390 and linux 32 bit builds broken
  • 4f9a1ff: 8254073: Tokenizer improvements (revised)
  • 9cecc16: 8254244: Some code emitted by TemplateTable::branch is unused when running TieredCompilation
  • ... and 445 more: https://git.openjdk.java.net/jdk/compare/c8257ea4e254bd4f8c574f781a6f6d8c658a1bca...master

Your commit was automatically rebased without conflicts.

Pushed as commit 52e45a3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

lewurm added a commit to lewurm/openjdk that referenced this pull request Oct 6, 2021
Restore looks like this now:
```
  0x0000000106e4dfcc:   movk    x9, #0x5e4, lsl openjdk#16
  0x0000000106e4dfd0:   movk    x9, #0x1, lsl openjdk#32
  0x0000000106e4dfd4:   blr x9
  0x0000000106e4dfd8:   ldp x2, x3, [sp, openjdk#16]
  0x0000000106e4dfdc:   ldp x4, x5, [sp, openjdk#32]
  0x0000000106e4dfe0:   ldp x6, x7, [sp, openjdk#48]
  0x0000000106e4dfe4:   ldp x8, x9, [sp, openjdk#64]
  0x0000000106e4dfe8:   ldp x10, x11, [sp, openjdk#80]
  0x0000000106e4dfec:   ldp x12, x13, [sp, openjdk#96]
  0x0000000106e4dff0:   ldp x14, x15, [sp, openjdk#112]
  0x0000000106e4dff4:   ldp x16, x17, [sp, openjdk#128]
  0x0000000106e4dff8:   ldp x0, x1, [sp], openjdk#144
  0x0000000106e4dffc:   ldp xzr, x19, [sp], openjdk#16
  0x0000000106e4e000:   ldp x22, x23, [sp, openjdk#16]
  0x0000000106e4e004:   ldp x24, x25, [sp, openjdk#32]
  0x0000000106e4e008:   ldp x26, x27, [sp, openjdk#48]
  0x0000000106e4e00c:   ldp x28, x29, [sp, openjdk#64]
  0x0000000106e4e010:   ldp x30, xzr, [sp, openjdk#80]
  0x0000000106e4e014:   ldp x20, x21, [sp], openjdk#96
  0x0000000106e4e018:   ldur    x12, [x29, #-24]
  0x0000000106e4e01c:   ldr x22, [x12, openjdk#16]
  0x0000000106e4e020:   add x22, x22, #0x30
  0x0000000106e4e024:   ldr x8, [x28, openjdk#8]
```
robehn pushed a commit to robehn/jdk that referenced this pull request Aug 15, 2023
pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024
pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024
pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants