RFR: 8341622: Tag-specific disabled default decorators for UnifiedLogging [v6]

[Axel Boldt-Christmas]

On Wed, 9 Oct 2024 09:13:37 GMT, Antón Seoane <duke at openjdk.org> wrote:

>> Currently, the Unified Logging framework defaults to three decorators (uptime, level, tags) whenever the user does not specify otherwise through -Xlog. However, some specific logging cases do not need decorations, and manually having to disable them results in cumbersome extra input and loss of ergonomics. For example, C2 developers rarely need decorations, and having to manually specify this every time results inconvenient.
>> 
>> To address this, this PR enables the possibility of adding tag-specific disabling of default decorators to UL. These disables are in no way overriding user input -- they will only act whenever -Xlog has no decorators supplied and there is a positive match with the pre-specified defaults. Such a match is based on an inclusion rule: e.g. if -Xlog:jit+compilation is provided, a default for jit may be applied. Additionally, defaults may target a specific log level.
>> 
>> The original use case for this is related to C2 logging migration to UnifiedLogging, as currently no decorators are found in compiler logs and it would be expected to stay the same without the extra explicit removal every time via -Xlog. However, this would ease the migration of other logging that was initially deterred by this, such as -XX:+PrintInterpreter.
>> 
>> This PR is a simplification of the [8340363](https://bugs.openjdk.org/browse/JDK-8340363) (closed) ticket.
>
> Antón Seoane has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Replace LogDecorators::Decorator::NoDecorator with LogDecorators::None

I have a few smaller nits, and one larger issue. The rest of the implementation and logic looks fine.

src/hotspot/share/logging/logDecorators.cpp line 30:

> 28: const LogLevelType AnyLevel = LogLevelType::NotMentioned;
> 29: #define UNDECORATED_DEFAULTS \
> 30:   UNDECORATED_DEFAULT(AnyLevel, LOG_TAGS(jit, inlining))

Maybe move this down to next where it is used and then `#undef UNDECORATED_DEFAULTS`

src/hotspot/share/logging/logDecorators.cpp line 55:

> 53: #define UNDECORATED_DEFAULT(level, ...) LogDecorators::DefaultUndecoratedSelection(level, __VA_ARGS__),
> 54:   UNDECORATED_DEFAULTS
> 55: #undef UNDECORATED_TAGSET

Suggestion:

#undef UNDECORATED_DEFAULT

Typo, I think this was ment to match with the `#define`.

src/hotspot/share/logging/logDecorators.cpp line 57:

> 55: #undef UNDECORATED_TAGSET
> 56: };
> 57: const size_t LogDecorators::number_of_default_decorators = sizeof(default_decorators) / sizeof(LogDecorators::DefaultUndecoratedSelection);

I think this reads better and is less error prone.
Suggestion:

const size_t LogDecorators::number_of_default_decorators = ARRAY_SIZE(default_decorators);

src/hotspot/share/logging/logDecorators.hpp line 142:

> 140:   // Check if we have some default decorators for a given LogSelection. If that is the case,
> 141:   // the output parameter mask will contain the defaults-specified decorators mask
> 142:   static bool has_disabled_default_decorators(const LogSelection& selection, const DefaultUndecoratedSelection* defaults = default_decorators, size_t defaults_count = number_of_default_decorators);

I was trying to think if we could make this mockable without the incomplete object types (`const DefaultUndecoratedSelection* defaults = default_decorators, size_t defaults_count = number_of_default_decorators`).  

Maybe have the mockable part private (we already friend the gtest) and only have a `static bool has_disabled_default_decorators(const LogSelection& selection)` public (which calls this on the inside). 

But I am fine with this as it currently is.

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

Changes requested by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21383#pullrequestreview-2367213509
PR Review Comment: https://git.openjdk.org/jdk/pull/21383#discussion_r1799860489
PR Review Comment: https://git.openjdk.org/jdk/pull/21383#discussion_r1799862505
PR Review Comment: https://git.openjdk.org/jdk/pull/21383#discussion_r1799845556
PR Review Comment: https://git.openjdk.org/jdk/pull/21383#discussion_r1799869065