On Fri, 9 May 2025 17:45:08 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> This is an alternative (and opposite) approach to
>> https://github.com/openjdk/jdk/pull/24895. We basically convert most `[cds]`
>> logs to `[aot]` logs. However, for the few logs that might be needed by
>> existing user scripts, we use macros like `aot_log_info`, `aot_log_debug` so
>> that they can be selected/printed using the `[cds]` tag.
>>
>> We have a few hundred logs that start with `[cds]`. To aid reviewing, this
>> PR will convert only part of them. I will create a second PR that coverts
>> the rest of the logs.
>>
>> Please see **aotLogging.hpp** for how the macros work.
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> More conversion; clean up; bug fixes
Some drive-by comments, not a full review as I find this all rather confusing
because I don't understand why we used "cds" in AOT specific code in the first
place. It makes perfect sense to me that AOT specific code logs to "aot".
It makes far less sense to me why you ever introduced logging that required
"cds" and "aot" in the first place, so getting rid of that is a good thing.
For code shared between cds and aot it is less clear to me how to proceed. If
the intent is that CDS will be no more and everything is AOT, then a transition
path is needed for the logging. I guess this is where your macros come in,
though I'm a bit unclear as to why it can be controlled by a user-defined
command-line flag rather than only by the CDS/AOT selection arguments operating
directly on an internal CDS/AOT configuration setting. ???
src/hotspot/share/cds/aotLogging.hpp line 69:
> 67: //
> 68: // $ java -Xlog:aot -XX:AOTCache=bad.aot ...
> 69: // [0.020s][info][cds] trying to map bad.jsa
Is it `bad.jsa` or `bad.aot`?
src/hotspot/share/cds/aotLogging.hpp line 77:
> 75: // - These logs can be selected ONLY with -Xlog:aot. They are always
> printed with [aot] decoration
> 76: //
> 77: // [2] When using CDS archives, and PrintAOTLogsAsCDSLogs=true
I guess I am missing some basic knowledge about AOT here. If we are using
legacy CDS not AOT, then why would there be any AOT log statements ???
src/hotspot/share/cds/aotLogging.hpp line 146:
> 144: LogTagSetMapping<LogTag::_aot, T1, T2, T3,
> T4>::tagset().vwrite(level, fmt, args);
> 145: }
> 146: }
```suggestion if possible
LogTag tag = PrintAOTLogsAsCDSLogs ? LogTag::_cds : LogTag::_aot;
LogTagSetMapping<tag, T1, T2, T3, T4>::tagset().vwrite(level, fmt, args);
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/25136#pullrequestreview-2835783884
PR Review Comment: https://git.openjdk.org/jdk/pull/25136#discussion_r2086245590
PR Review Comment: https://git.openjdk.org/jdk/pull/25136#discussion_r2086248325
PR Review Comment: https://git.openjdk.org/jdk/pull/25136#discussion_r2086253167
