RFR: 8292269: Log more CDS failure messages in the warning channel

[Matias Saavedra Silva]

On Mon, 6 Feb 2023 04:50:41 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> When -Xshare:auto is used, FileMapInfo::fail_continue() prints the diagnostic message to the Info channel instead of Warning. This was necessary in the past because CDS would failed to load for many reasons (e.g., failure to mmap due to ASLR) that are not the fault of the user. If we printed the message in the Warning channel, the user would be overwhelmed and would ultimately ignore the warnings, rendering them useless.
>> 
>> However, currently CDS is much more reliable. For some error conditions that require user attention, we should consider changing the failure logs to use the Warning channel.
>> 
>> Since only the "warning" and "info" channels are used, they can each have their own methods where fail_continue() defaults to the warning channel. Some messages must remain in the "info" channel as their inclusion would make the output too verbose. Verified with tier1-4 tests
>
> src/hotspot/share/cds/filemap.cpp line 118:
> 
>> 116: }
>> 117: 
>> 118: void FileMapInfo::fail_continue_nowarn(const char *msg, ...) {
> 
> It is better to name a method by what it does do rather than what it doesn't. 
> 
> But I am finding the naming here problematic anyway as we do not always continue - the logic is basically `possibly_fail_else_log_and_continue`. Maybe the simplest thing here is to only have the variant that takes the `LogLevel` and so every callsite is explicit about whether it logs at warning or info level rather than try to accommodate warn/info into the name somehow?

I agree that the `fail_continue` is not the ideal name for what this is trying to accomplish: log a message and *possibly* fail. 

That being said, I felt it was appropriate to abstract away the need to manage `LogLevel` as only Info and Warning are used with `fail_continue`. If someone does want to use any of the other channels, like debug, error, etc, they can still use the `fail_continue` variant that excepts a `LogLevel`, but they wouldn't have to worry about it otherwise. I think the binary option of warning vs no warning can help to keep it simple.

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

PR: https://git.openjdk.org/jdk/pull/12419