8354362: Use automatic indentation in CollectedHeap printing

[jsikstro]

Hello,

This PR only focuses on fixing indentation and re-arranging some callsites. It does not change the contents of any output, apart from some (IMO relevant) indentation/whitespace additions.

Update: With some suggestions from @stefank, I've renamed print_on to print_heap_on and print_on_error to print_gc_on to better reflect their purpose where they're called. With this I've also renamed other instances of print_on_error to better reflect their purpose. Printing heap information and printing gc information is now two distinct steps in vmError.cpp.

Currently, the CollectedHeap printing code (print_on and print_on_error, with calls "below") prepends spaces in messages in a way that only makes sense if you write the code and then check the output to see if you've done everything correctly. To make writing and maintaining printing code easy, I propose we move to a system where each printing method, starting at callers of print_on and print_on_error, uses the indentation API in outputStream and does not rely on prepending spaces like is done right now.

What I propose is that any (GC) printing method should not make any assumptions of the indentation level of its caller(s). This means that each function shall:

1. Not prepend any spaces to its printing, and instead expect that the caller(s) should handle any indentation before calling this function.
2. Enforce its own indentation, by enabling auto indentation in its own context and for its "lower level" calls (which is often the desired outcome).

Combining these two rules means that any (GC) printing method can be called from anywhere and give sensible output, without (seemingly random) indentation of expectations elsewhere.

I have aggregated calls that print on the same indentation level to the same callsite. This makes it clear where to look in the code and also makes it easier to add/enforce indendation. To this end, I have re-arranged print_on_error so that it never includes print_on. The new system I propose is that print_on and print_on_error can be called separately for different information, which aligns well with having the same callsite for the same indentation. See changes in vmError.cpp for how this is implemented.

Instead of prepending spaces, I use StreamAutoIndentor, defined in ostream.hpp. To make using automatic indentation easier, I've made some changes to StreamAutoIndentor so that it inherits from streamIndentor and also add an optional argument to StreamAutoIndentor to apply an indentation. My reasoning for this is that most places that use streamIndentor also want to use StreamAutoIndentor (either immediately or some time before) so that it is automatically applied. A downside of this change is that any previous uses of StreamAutoIndentor now also needs to store an extra int worth of memory. To me, this is a trade-off worth making, considering that memory for buffers of strings usually outweigh this extra memory cost. Additionally, when factoring in the improved code understandability and maintainability, I feel like it's a change worth making.

Some new changes in the way the printing looks are:

• Epsilon has received indentation in its print_on, which was not there before, in an effort to look similar to other GCs and also improve readability.
• Shenandoah has also received indentation to behave similar to other GCs.
• "the space" in Serial's output was indented by two spaces, now it's one.
• With the removal of print_on from print_on_error, I've also removed Epsilon's barrier set printing, making it's print_on_error empty. Before this, Serial printed two spaces between the sections in the hs_err file.

Code re-structure:

• PSOldGen::print_on had an inlined version of virtual_space()->print_space_boundaries_on(st), which is now called instead.
• PSYoungGen::print_on had its name inlined. Now, name() is called instead, which is how PSOldGen::print_on does it.
• I've added a common print_space_boundaries_on for the virtual space used in Serial's DefNewGeneration and TenuredGeneration, like how Parallel does it.
• I've opted to use fill_to() in Metaspace printing so that it works well with ZGC printing. This does not really affect other GCs since only ZGC aligns with the same column as Metaspace.

Testing:

• GHA, Oracle's tier 1-3
• Manual inspection of printed content
  • Exit printing -Xlog:gc+heap+exit=info
  • Periodic printing -Xlog:gc+heap=debug
  • jcmd jcmd <pid> GC.heap_info
  • jcmd jcmd <pid> VM.info
  • hs_err file, both "Heap:" and "Heap before/after invocations=" printing, -XX:ErrorHandlerTest=14

---

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-8354362: Use automatic indentation in CollectedHeap printing (Enhancement - P4)

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer) Review applies to 33d20641
• Stefan Karlsson (@stefank - Reviewer)
• Leo Korinth (@lkorinth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24593

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jsikstro! 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]

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

8354362: Use automatic indentation in CollectedHeap printing

Reviewed-by: stefank, lkorinth, 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 70 new commits pushed to the master branch:

• 0537c69: 8355492: MissedOptCastII is missing UnlockDiagnosticVMOptions flag
• 34c4d89: 8353190: Use "/native" Run Option for TestAvailableProcessors Execution
• 1ec6481: 8350582: Correct the parsing of the ssl value in javax.net.debug
• ... and 67 more: https://git.openjdk.org/jdk/compare/9eeb86d972ac4cc38d923b2b868b426bbd27a4e8...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]

@jsikstro The following labels will be automatically applied to this pull request:

• hotspot
• serviceability
• shenandoah

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 pull request command.

[jsikstro]

/label remove serviceability
/label remove hotspot
/label add hotspot-gc

[openjdk]

@jsikstro
The serviceability label was successfully removed.

[openjdk]

@jsikstro
The hotspot label was successfully removed.

[openjdk]

@jsikstro
The hotspot-gc label was successfully added.

[jsikstro]

Ping @tstuefe regarding changes for StreamAutoIndentor. Would be nice to get your opinion since you are the author of it and its uses :)

[mlbridge]

Webrevs

• 13: Full - Incremental (daf58f9d)
• 12: Full - Incremental (a9dc76a3)
• 11: Full - Incremental (b9b975de)
• 10: Full - Incremental (e89b916f)
• 09: Full - Incremental (1cb525aa)
• 08: Full - Incremental (115dc41a)
• 07: Full (1e9d4be4)
• 06: Full - Incremental (042c0aee)
• 05: Full - Incremental (0824712c)
• 04: Full - Incremental (33d20641)
• 03: Full - Incremental (2979316c)
• 02: Full (c1140b86)
• 01: Full - Incremental (9fea46ab)
• 00: Full (2c0c0b2b)

[tstuefe]

Hi @jsikstro, good cleanup, some small nits remain.

Cheers, Thomas

[tstuefe]

Notes:

• We may want to simplify at some point and merge streamIndentor and streamAutoIndentor. That includes checking which existing call sites use streamIndentor without wanting auto indentation. Not sure but I guess there are none.
  I think the existing cases fall into two categories: where streamIndentor was used on a stream that had already autoindent enabled, and where the code uses "cr_indent()" or "indent" to manually indent.

• It would be nice to have a short comment in collectedHeap.hpp about when print_on resp print_on_error is used. From your explanation, I expect print_on_error to be used for information that should only be printed in case of a fatal error, right?

• To simplify and prevent mistakes, we should consider making set_autoindent in outputStream private and make the indentor RAII classes friends of outputStream.

[openjdk]

@jsikstro Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jsikstro]

Sorry for the force-push, made a mistake when merging with master. No comments should have been removed.

[jsikstro]

Thank you for looking at this @tstuefe!

I've addressed some of your comments with new commits. I agree that we likely want to merge streamIndentor and StreamAutoIndentor in a follow up RFE, where it also would be good to look at making set_autoindent() private.

I haven't looked into it, but it feels weird to have an indentation level on an outputStream and use it only explicitly via indent() and not via a StreamAutoIndentor. I think a good solution would be to only allow indentation via the StreamAutoIndentor API like you're proposing, and look into whether there should be some API for temporarily disabling indentation with a RAII object (or just some parameters to StreamAutoIndentor) if there are cases that require it.

[jsikstro]

After some discussion with @lkorinth and @xmas92 I've opted to use consistent line-breaking and some other style changes.

[lkorinth]

Just a note, not a request for change: I would personally have used composition instead of inheritance here (of streamIndentor).

[lkorinth]

I think this looks great. Thank you.

You might want to run a bit more internal testing; it is too easy to write a test that looks for specific formatting in the logs.

Mostly unrelated to this change: I think that outputStream probably should have a version of fill_to that guarantees a separating space so that a trailing space does not need to be put before the call (it ought to be by far the most common case). That is clearly out of scope for this change however.

[stefank]

Looks good.

One suggestion:

[lkorinth]

lgtm

[jsikstro]

Thank you for the reviews!
/integrate

[openjdk]

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

• 0537c69: 8355492: MissedOptCastII is missing UnlockDiagnosticVMOptions flag
• 34c4d89: 8353190: Use "/native" Run Option for TestAvailableProcessors Execution
• 1ec6481: 8350582: Correct the parsing of the ssl value in javax.net.debug
• ... and 67 more: https://git.openjdk.org/jdk/compare/9eeb86d972ac4cc38d923b2b868b426bbd27a4e8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jsikstro Pushed as commit cf96b10.

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