RFR: 8355692: Refactor stream indentation [v3]
Joel Sikström
jsikstro at openjdk.org
Mon May 12 12:45:20 UTC 2025
> Hello,
>
>> To make it easier to review this PR, each commit that is prefixed with "FIX:" has the command/method I've used to verify that the output of prints is not changed/broken.
>
> The goal of this PR is to refactor the usage of streamIndentor and StreamAutoIndentor and combine them into a single StreamIndentor class to make it easier to apply indentation. [JDK-8354362](https://bugs.openjdk.org/browse/JDK-8354362) introduced a partial solution for the refactor I propose, by being able to add indentation with a StreamAutoIndentor.
>
> To use indentation, you currently need to do two separate things: 1) increment indentation and 2) apply indentation when printing. The indentation level can be incremented by using one of: streamIndentor, inc() or inc(int n), and automatic indentation can be enabled by using one of: StreamAutoIndentor, indent(), cr_indent().
>
> The new StreamIndentor has the functionality of both streamIndentor and StreamAutoIndentor, that is, enabling automatic indentation and also applying an optional amount of indentation. This means you only need StreamIndentor to make sure that indentation is incremented and applied.
>
> There are currently four (4) ways that indentation is applied in HotSpot:
>
> 1) The new method of enabling automatic indentation and applying indentation simultaneously (partially implemented already).
> Only in GC printing code via CollectedHeap.
>
> 2) Initially use a StreamAutoIndentor, then use streamIndentor to temporarily increment indentation. Indentation is automatically applied when printing.
> nmt/memReporter uses this principle, by having a StreamAutoIndentor as a member variable and applying indentation via streamIndentor when needed.
>
> 3) Use a streamIndentor and manually call indent() (or cr_indent()).
> Commonly used pattern. No need to manually apply indentation if automatically applied.
>
> 4) Increment/decrement indentation (using inc() and/or dec()) and manually call indent().
> Only C1's CFGPrinterOutput does this.
>
> I leave the fourth case alone as it is fairly self-contained and requires a more extensive re-write, appropriate for a follow-up patch if that's what we want.
>
> When refactoring, I only found a single case where a streamIndentor couldn't be trivially replaced with the new StreamIndentor. All other streamIndentors, along with manually applied indentation using indent()/cr_indent(), are able to be replaced with the new StreamIndentor without changing the output.
>
> After the changes in this PR there are only two ways th...
Joel Sikström has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
- Merge branch 'master' into JDK-8355692_stream_refactor
- Make sure indentation is unchanged in ArenaStats::print_on
- Remove cr_indent()
- Adapt gtest
- Make outputStream::set_autoindent private
- Rename to StreamIndentor si[level]
- Combine streamIndentor and StreamAutoIndentor
- FIX: memReporter
Verified with command: jcmd <lingering program> VM.native_memory
- FIX: compilationMemoryStatistic.cpp
Verified with command: java -XX:CompileCommand=memstat,\*.\*,print App.java
- FIX: Trivial changes in compilationMemoryStatistic.cpp
- ... and 8 more: https://git.openjdk.org/jdk/compare/6c030906...b71db0dd
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/24917/files
- new: https://git.openjdk.org/jdk/pull/24917/files/5c4bd960..b71db0dd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=24917&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=24917&range=01-02
Stats: 51159 lines in 1558 files changed: 35130 ins; 8681 del; 7348 mod
Patch: https://git.openjdk.org/jdk/pull/24917.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/24917/head:pull/24917
PR: https://git.openjdk.org/jdk/pull/24917
More information about the hotspot-dev
mailing list