RFR: 8229517: Support for optional asynchronous/buffered logging [v2]

[Xin Liu]

On Fri, 26 Mar 2021 09:12:05 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Xin Liu has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - 8229517: Support for optional asynchronous/buffered logging
>>    
>>    add a constraint for the option LogAsyncInterval.
>>  - 8229517: Support for optional asynchronous/buffered logging
>>    
>>    LogMessage supports async_mode.
>>    remove the option AsyncLogging
>>    renanme  the option GCLogBufferSize to AsyncLogBufferSize
>>    move drop_log() to LogAsyncFlusher.
>
> src/hotspot/share/logging/logFileOutput.cpp line 322:
> 
>> 320: 
>> 321:   LogAsyncFlusher* flusher = LogAsyncFlusher::instance();
>> 322:   if (_async_mode && flusher != NULL) {
> 
> Why you don't check for `flusher == NULL` in `LogAsyncFlusher::instance()` and call `LogAsyncFlusher::initialize()` in case it is NULL. I think it's no difference where the NULL check is and doing it in `LogAsyncFlusher::instance()` will save you from calling `LogAsyncFlusher::initialize()` in `init_globals()`.
> 
> Put `LogAsyncFlusher::instance()` into the `if (_async_mode)` block and add an assertion that `flusher != NULL`.

hi, @simonis  and @tstuefe 

The purpose of nullcheck here is to circumvent the problem of circular dependency which also mentioned by Thomas.  Logging subsystem as a low-level infrastructure could be invoked as early as `LogConfiguration::initialize` has done in `Threads::create_vm()`. At this time, mutex and Thread haven't been initialized, therefore, I can't initialize `LogAsyncFlusher` appropriately. 

The reason I provide separated APIs `LogAsyncFlusher::initialize()` & `LogAsyncFlusher::instance()` because I need to separate log subsystems into 2 phrases. LogAsyncFlusher::initialize() marks an important point. Before this point, logging subsystem can only use synchronous mode no matter what. After this point, async_mode takes effect because buffer has been established.  That is to say, all code in this PR can only take over logging after that point.  

I read Thomas concerned about fancy calls such as strdup, os::malloc etc.  Instead of designing async-logging absolutely self-contained and low-level, I chose a common technique called 'lazy-initialization'.  I don't think async mode is absolutely necessary in the startup time of hotspot. The gain is obvious. I can use hotspot's established constructs to simplify the implementation. 

May I ask you and Thomas to comment on this design? If it works, I will add more comments about it.

> test/hotspot/gtest/logging/test_asynclog.cpp line 168:
> 
>> 166:   EXPECT_FALSE(file_contains_substring(TestLogFileName, "log_trace-test")); // trace message is masked out
>> 167:   EXPECT_TRUE(file_contains_substring(TestLogFileName, "log_debug-test"));
>> 168: }
> 
> Should have a newline at the end.

noted.

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

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