RFR: 8286117: Remove unnecessary indirection and unused code in UL

[Johan Sjölén]

On Thu, 5 May 2022 05:56:15 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This PR cleans up some minor annoyances in the UL code.
>> 
>> Note that the change from `LogImpl` to `LogTagSet` is equivalent, we're essentially inlining the calls that `LogImpl` would have made. This simplifies understanding ("why do we need `LogImpl` here?" was my question, answer: We don't!)'
>
> src/hotspot/share/logging/log.hpp line 184:
> 
>> 182: 
>> 183:   static bool is_enabled() {
>> 184:     return LogTagSetMapping<T0, T1, T2, T3, T4, GuardTag>::tagset().is_level(level);
> 
> This removes an abstraction and makes the code more verbose. I don't see the value of doing this, especially when LogImpl is used by macros like `log_info()`, `log_debug()`, etc, which are the primary interface to UL. With this change, people who wonder how `log_info()` works would need to understand more implementation details than before.

Hi @iklam, `LogImpl` is still used in `log_X()` macros! `LogTarget` macro uses `LogTargetImpl`, which this changes. We're not lowering the level of abstraction, we're getting rid of an unneeded one that, to me, made the code even more difficult to understand.

Let's read the code together, because this was confusing to me when I saw it.


LogImpl<T0, T1, T2, T3, T4, GuardTag>::is_level(level) ~> // log.hpp:186:LogImplTarget
LogTagSetMapping<T0, T1, T2, T3, T4>::tagset().is_level(level) ~> // log.hpp:124:LogImpl
_output_list.is_level(level); // logTagSet.hpp:126:LogTagSet


When you read `log.hpp:186` you're probably thinking "OK, `LogTargetImpl` *needs* `LogImpl`, which makes sense!" but then you follow the code and LogImpl doesn't provide any extra state for `LogTagSet`. So maybe then you think that `LogImpl` provides some extra state when constructed (nope, empty constructor, LogTagSetMapping constructs LogTagSet for us statically).

At about that point I made the conclusion that this indirection is unneeded and should be removed :-).

PS. Accidentally posted this from my *other* Github account, so sorry for the double notification e-mail :-).

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

PR: https://git.openjdk.java.net/jdk/pull/8541