8325945: Error reporting should limit the number of String characters printed #20150

dholmes-ora · 2024-07-12T02:17:46Z

Please review this enhancement that intends to improve the readability of error logs when very long java.lang.Strings exist and when printed in full they obscure things in the log.

The suggestion was to add a MaxStringPrintSize flag, similar to the MaxElementPrintSize for arrays. I've set the default to 256 (arbitrary selection: not too big, not too small - may need adjusting) with a range from 2 to O_BUFLEN.

The method java_lang_String::print now takes a max_length parameter that defaults to MaxStringPrintSize. This allows more direct control if specific call sites want to print full strings regardless.

If a string's length exceeds max_length then we print it as follows:

"< first max_length/2 characters> ... <last max_length/2 characters>" (abridged)

For example if we print "ABCDE" with a max_length of 4 then the output is literally:

"AB ... DE" (abridged)

The message doesn't mention MaxPrintStringSize as that may not be involved in limiting the printed length. Developers will need to know to look at that (which is not 100% satisfactory but explaining everything in the output itself seems a bit excessive).

For testing purposes I added a WhiteBox API to print the string to a stringStream and then return it as a new java.lang.String.

Testing:

new test added for validation purposes
tiers 1 - 3 as sanity testing

Thanks

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8325945: Error reporting should limit the number of String characters printed (Bug - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20150/head:pull/20150
$ git checkout pull/20150

Update a local copy of the PR:
$ git checkout pull/20150
$ git pull https://git.openjdk.org/jdk.git pull/20150/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20150

View PR using the GUI difftool:
$ git pr show -t 20150

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20150.diff

Webrev

Link to Webrev Comment

… printed

bridgekeeper · 2024-07-12T02:18:36Z

👋 Welcome back dholmes! 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 · 2024-07-12T02:20:12Z

@dholmes-ora 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:

8325945: Error reporting should limit the number of String characters printed

Reviewed-by: thartmann, stuefe

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

35df48e: 8335860: compiler/vectorization/TestFloat16VectorConvChain.java fails with non-standard AVX/SSE settings
7bf5313: 8335480: Only deoptimize threads if needed when closing shared arena
1b83bd9: 8336661: Parallel: Remove stacks_empty assert in PSScavenge::invoke
72297d2: 8317720: RISC-V: Implement Adler32 intrinsic
21a6cf8: 8336587: failure_handler lldb command times out on macosx-aarch64 core file
78cc0f9: 8336091: Fix HTML warnings in the generated HTML files
bcb5e69: 8335921: Fix HotSpot VM build without JVMTI
10186ff: 8336300: DateFormatSymbols#getInstanceRef returns non-cached instance
7ec55df: 8336638: Parallel: Remove redundant mangle in PSScavenge::invoke
6df7acb: 8299080: Wrong default value of snippet lang attribute
... and 56 more: https://git.openjdk.org/jdk/compare/81a0d1ba03bbdbe718302b3925cdc207d5d05232...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-07-12T02:20:38Z

@dholmes-ora The following label will be automatically applied to this pull request:

hotspot

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

mlbridge · 2024-07-12T02:23:05Z

Webrevs

TobiHartmann

Looks good to me otherwise. Thanks for fixing this!

src/hotspot/share/runtime/globals.hpp

src/hotspot/share/classfile/javaClasses.cpp

dholmes-ora · 2024-07-17T05:29:36Z

Thanks for the review @TobiHartmann

@tstuefe

…sted by @tstuefe

dholmes-ora · 2024-07-18T06:45:56Z

I've updated per Thomas's suggestion to show the number of characters omitted. However I kept the " (abridged)" part as well as with actual very long strings you are more likely to spot that at the end than the "... (N characters ommitted) ..." in the middle.

tstuefe · 2024-07-18T07:23:16Z

src/hotspot/share/classfile/javaClasses.cpp

+
+  if (length > max_length) {
+    st->print(" (abridged) ");
+  }


Do we still need this marker?

See my comment above:
#20150 (comment)

Oh that makes sense. Okay!

tstuefe · 2024-07-18T07:24:18Z

src/hotspot/share/runtime/globals.hpp

+          "in the VM. If exceeded, an abridged version of the string is "   \
+          "printed with the middle of the string elided.")                  \
+          range(2, O_BUFLEN)                                                \
+                                                                            \


This would make sense as a product diagnostic switch. You want to be able to increase this at a customer if needed.

This is modelled after the MaxElementPrintSize that precedes it.

tstuefe

Looks good.

TobiHartmann

Still looks good to me.

dholmes-ora · 2024-07-19T06:21:40Z

Thanks for the reviews!

/integrate

openjdk · 2024-07-19T06:23:14Z

Going to push as commit 10fcad7.
Since your change was applied there have been 78 commits pushed to the master branch:

f5871df: 8336675: Remove UnixFileSystemProvider.checkPath in favor of UnixPath.toUnixPath
902bada: 8234071: JTable.AUTO_RESIZE_LAST_COLUMN acts like AUTO_RESIZE_ALL_COLUMNS
1b9270a: 8328723: IP Address error when client enables HTTPS endpoint check on server socket
330e520: 8028127: Regtest java/security/Security/SynchronizedAccess.java is incorrect
39f4476: 8334772: Change Class::signers to an explicit field
902c2af: 8336585: BoundAttribute.readEntryList not type-safe
b44632a: 8336588: Ensure Transform downstream receives upstream start items only after downstream started
02be7b8: 8334495: Use FFM instead of jdk.internal.misc.Unsafe in java.desktop font implementation
bbc79a5: 8333768: Minor doc updates to java.lang.{Float, Double}
245c086: 8332600: javac uses record components source position during compilation
... and 68 more: https://git.openjdk.org/jdk/compare/81a0d1ba03bbdbe718302b3925cdc207d5d05232...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-19T06:23:19Z

@dholmes-ora Pushed as commit 10fcad7.

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

DamonFool · 2024-07-19T11:54:12Z

runtime/PrintingTests/StringPrinting.java fails with release VMs.
Please see #20249
Thanks.

dholmes-ora added 4 commits July 11, 2024 19:12

8325945: Error reporting should limit the number of String characters…

17a91f8

… printed

Merge branch 'master' into 8325945-print-string

86f8ebf

Fix indent

934f8e3

Improve flag description

7b155ab

openjdk bot added the rfr Pull request is ready for review label Jul 12, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jul 12, 2024

TobiHartmann approved these changes Jul 17, 2024

View reviewed changes

src/hotspot/share/runtime/globals.hpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jul 17, 2024

tstuefe reviewed Jul 17, 2024

View reviewed changes

src/hotspot/share/classfile/javaClasses.cpp Show resolved Hide resolved

Fixed grammar

39256bd

openjdk bot removed the ready Pull request is ready to be integrated label Jul 17, 2024

Change output format to show number of characters ommitted as a sugge…

6d445db

…sted by @tstuefe

dholmes-ora requested a review from TobiHartmann July 18, 2024 06:46

Update comment

e45fb90

tstuefe reviewed Jul 18, 2024

View reviewed changes

tstuefe approved these changes Jul 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 18, 2024

TobiHartmann approved these changes Jul 18, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 19, 2024

openjdk bot closed this Jul 19, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 19, 2024

dholmes-ora deleted the 8325945-print-string branch July 21, 2024 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8325945: Error reporting should limit the number of String characters printed #20150

8325945: Error reporting should limit the number of String characters printed #20150

dholmes-ora commented Jul 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 12, 2024

openjdk bot commented Jul 12, 2024 •

edited

Loading

openjdk bot commented Jul 12, 2024

mlbridge bot commented Jul 12, 2024 •

edited

Loading

TobiHartmann left a comment

dholmes-ora commented Jul 17, 2024

dholmes-ora commented Jul 18, 2024

tstuefe Jul 18, 2024

dholmes-ora Jul 18, 2024

tstuefe Jul 18, 2024

tstuefe Jul 18, 2024

dholmes-ora Jul 18, 2024

tstuefe left a comment

TobiHartmann left a comment

dholmes-ora commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

DamonFool commented Jul 19, 2024

8325945: Error reporting should limit the number of String characters printed #20150

8325945: Error reporting should limit the number of String characters printed #20150

Conversation

dholmes-ora commented Jul 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 12, 2024

openjdk bot commented Jul 12, 2024 • edited Loading

openjdk bot commented Jul 12, 2024

mlbridge bot commented Jul 12, 2024 • edited Loading

Webrevs

TobiHartmann left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jul 17, 2024

dholmes-ora commented Jul 18, 2024

tstuefe Jul 18, 2024

Choose a reason for hiding this comment

dholmes-ora Jul 18, 2024

Choose a reason for hiding this comment

tstuefe Jul 18, 2024

Choose a reason for hiding this comment

tstuefe Jul 18, 2024

Choose a reason for hiding this comment

dholmes-ora Jul 18, 2024

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

DamonFool commented Jul 19, 2024

dholmes-ora commented Jul 12, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 12, 2024 •

edited

Loading

mlbridge bot commented Jul 12, 2024 •

edited

Loading