RFR: 8333211: NMT Reports: replace manual indentation handling with auto indent
David Holmes
dholmes at openjdk.org
Tue Jun 4 06:37:04 UTC 2024
On Wed, 29 May 2024 16:10:07 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> Hi,
>
> this is the first of a few patches I plan to reduce NMT code complexity.
>
> In NMT reporting, indentations are manually hard-coded. That makes the code brittle and frustrating to maintain. One cannot, e.g., re-use a printing subfunction in differently aligned portions without some awkward gymnastics (e.g. handing indentation as argument to the sub routine).
>
> This patch adds a simple automatic indentation feature to `outputStream` and then uses that feature to simplify NMT reporting. NMT reporting becomes more malleable and easier to change.
>
> I plan to reuse this feature for a couple of non-NMT things.
>
> ---
>
> The new "autoindent" mode in `outputStream` is off by default. If enabled, the stream automatically adds indentations at new lines.
>
> The added code is small and minimally invasive. It builds atop the existing manual indentation. It trades completeness for simplicity:
> - it only works for a selected set of APIs (`print`, `print_cr`, `print_raw`, `print_raw_cr`).
> - it does not work for multiline output: `print("line\nline\nline")` needs to be broken up into individual `print` calls
>
> Despite these limitations, the feature works fine and I expect covers 99% of all use cases.
>
> In the NMT reports, I replaced the many manual indentations with a few incarnations of `streamIndentor`. The changes in detail:
>
> - to prevent backward compatibility issues, I am careful to limit the autoindent mode to NMT reporting.
> - I replaced manual indentation with a few `streamIndentor` placements
> - I removed the now useless `print_(malloc|virtual_memory|arena)_line`
>
> - Some minor cleanups:
> - merged the `outputStream` constructors into one
> - moved indentation level and autoindent flag to the private section of `outputStream`
> - removed some unused accessors in `outputStream`
> - tightened printing code in NMT reporting, e.g. replaced `print_cr(" ")` with `cr()`
> - removed the now unused `NativeCallStack::print_on(outputStream* out, int indent)` which hands in indentation.
>
> The last point I see as the real benefit of autoindent; we now don't have to manually hand indentation levels to printing subroutines. Subroutines can be oblivious to indentation - their printouts will be correctly indented regardless of indentation level.
>
> -----
>
> Testing:
>
> - GHAs
> - I ran NMT jtreg tests manually
> - I was careful to preserve the formatting of the existing reports. Therefore I tested all four variants: summary, summary.diff, detail, detail.diff , compared with outputs from mainline, an...
Hi Thomas,
I'm only looking at the auto-indent aspect of this as I'm also working on the stream code and reviewing another change using indentation. To be honest I cannot understand how outputstream's indentation works because to me indentation is always/only something that applies at the start of a new line - hence "auto-indent" would always be the case. I mean "indentation" in the middle of a line is a meaningless concept, so the logic in `indent` has me really bemused.
src/hotspot/share/utilities/ostream.hpp line 83:
> 81: void dec(int n) { _indentation -= n; };
> 82: int indentation() const { return _indentation; }
> 83: void set_indentation(int i) { _indentation = i; }
Why have you deleted these?
src/hotspot/share/utilities/ostream.hpp line 106:
> 104: void print_raw(const char* str, size_t len);
> 105: void print_raw_cr(const char* str) { print_raw(str); cr(); }
> 106: void print_raw_cr(const char* str, size_t len) { print_raw(str, len); cr(); }
Can these be simplified if we have a default value for len? i.e.
void print_raw(const char* str, size_t len = strlen(str));
-------------
PR Review: https://git.openjdk.org/jdk/pull/19461#pullrequestreview-2095410597
PR Review Comment: https://git.openjdk.org/jdk/pull/19461#discussion_r1625408785
PR Review Comment: https://git.openjdk.org/jdk/pull/19461#discussion_r1625413709
More information about the hotspot-runtime-dev
mailing list