RFR: JDK-8317683: Add JIT memory statistics [v3]

[Andrew Dinn]

On Thu, 12 Oct 2023 15:54:11 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/compiler/compilerOracle.hpp line 65:
>> 
>>> 63:   option(PrintInlining, "PrintInlining", Bool) \
>>> 64:   option(PrintIntrinsics, "PrintIntrinsics", Bool) \
>>> 65:   option(PrintMemStat, "PrintMemStat", Bool)   \
>> 
>> Ok, I still think it would be more coherent for users to have a single 3-way option
>> 
>>   MemStat=Off/Collect/Print  # or None instead of Off or whatever
>> 
>> I looked further into how the option validation works and it seems it would be possible to provide a single configuration option MemStat of type Ccstr to implement this. In order for it to work you would need to employ a validator in method scan_value (file compilerOracle.cpp:635) for the case when a MemStat option turns up during Ccstr option processing -- very like what happens for Ccstrlist options such as ControlIntrinsic. If the string text was not one of the 3 legitimate options it would be rejected.
>> Your methods which test whether to collect or print would need to be tweaked accordingly to check the string value (or probably just its first element).
>> I'll leave it up to you whether or not you want to make this change.
>
> I would like to leave it as it is:
> 
> I managed to do what you advised: use cstr for an enum-like option "memstat" with a value parser that accepts (and canonicalizes) "Print" and "Collect". 
> 
> I also managed to give it a default *value*, since that did not work before - so, omitting the value would equal to empty string which would equal some reasonable value default for the option.
> 
> One remaining problem was that I needed a quick way to find out if, for any method, the print option had been set. I need this at VM shutdown - if any directive exists that caused printing, we want to print the final report. Here I either would have to scan through all directives, or remember the state in a global flag like `CompilerOracle::_quiet`. It is all possible and solvable, but at this point, I would like to give up and wrap up this patch. 
> 
> We can re-visit this topic at a later date maybe by adding support for enums. There are other option combinations that would benefit being turned into an enum too, e.g. `NoRTMLockEliding` and `UseRTMLockEliding`.
> 
> One minor aesthetic point is that "PrintMemStat" fits nicely with the other "PrintXXX" options, and feels like something every compiler dev knows.
> 
> Thanks again for the review.

Thanks for trying. My concern was merely that this is UI providing two separate controls and it breaks the general usability maxim that one should not provide separate controls for things that are dependent. We can, of course, revise the UI later.

As far as the final printing pass is concerned could you not simply increment a counter (or simply just set a boolean) each time a MemStat=Print option is parsed? or exercised?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16076#discussion_r1357068712