RFR: 8333211: NMT Reports: replace manual indentation handling with auto indent

[Thomas Stuefe]

On Tue, 4 Jun 2024 06:19:35 GMT, David Holmes <dholmes 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: ...
>
> 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));

Interesting question. I try to avoid non-const default arguments since I never can recall their restrictions. 

Looked it up, I think it is not possible to refer to other function arguments in default values:


  A default argument is evaluated each time the function is called with no argument for the corresponding  
  parameter. Function parameters are not allowed in default arguments except if they are [not evaluated
  (https://en.cppreference.com/w/cpp/language/expressions#Potentially-evaluated_expressions). Note that
  parameters that appear earlier in the parameter list are in [scope
  (https://en.cppreference.com/w/cpp/language/scope):

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19461#discussion_r1625511137