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

[Xin Liu]

On Thu, 25 Mar 2021 19:30:25 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Xin Liu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
>> 
>>  - Refactor LogAsyncFlusher::abort()
>>    
>>    This change makes sure LogAsyncFlusher::abort() is lock-less.
>>    Therefore, it's not subject to mutex rank issue. Newly added
>>    gtest(mutex_lock_access_leaf)  may deliberately trigger SIGSEGV
>>    while holding access rank mutex, so abort() must be lockless.
>>  - Merge branch 'master' into JDK-8229517
>>  - Fix a race condition bug on LogAsyncFlusher termination.
>>    
>>    I saw intermitent crashes of java with the following arguments.
>>    -Xlog:'all=trace:file=hotspot-x.log:level,tags:filecount=0,async=true' --version
>>    
>>    The root cause is that there is a race condition between main thread _exit and
>>    LogAsyncFlusher::run. This patch added a synchronization using
>>    Terminator_lock in LogAsyncFlusher::terminate. This guranatees that no
>>    log entry will emit when main thread is exiting.
>>  - Resolve rank conflict between tty_lock and LogAsyncFlusher's _lock.
>>    
>>    LogAsyncFlusher::_lock ranks Mutex::tty on purpose, which is same as tty_lock.
>>    Ideally, they are orthogonal. In reality, it's possible that a thread emits logs
>>    to a log file while (mistakenly) holding tty_lock. ttyUnlocker is placed in enqueue
>>    member functions to resolve conflicts betwen them.
>>    
>>    This patch fixed the jtreg test: runtime/logging/RedefineClasses.java and the
>>    full brunt logging -Xlog:'all:file=hotspot.log::async=true'
>>  - Remove LogAsyncInterval completely
>>    
>>    It was used as a timeout parameter of the monitor. Now the monitor is waked up
>>    when the occupancy of asynclog buffer is more 3/4.
>>  - Support Jcmd pid VM.log disable
>>    
>>    This patch also supports to add a new output dynamically. If
>>    output_option specifies async=true, the new output will use
>>    asynchronous writing.
>>    
>>    Currently jcmd VM.log prohibts users from changing established
>>    output_options in runtime. users can disable them all and then
>>    recreate them with the new output_options.
>>  - Fix build issue with `--disable-precompiled-headers`
>>  - Inject the number of dropped messages since last dumpping.
>>    
>>    Each LogOutput has an independent counter. The out-of-band message
>>    "[number] messages dropped..." will be dumped into its corresponding
>>    LogOutput.
>>  - Revert "fix runtime/logging/RedefineClasses.java crashed with -XX:+AsyncLogging"
>>    
>>    This reverts commit 81b2a0cb2a6cf57b1cd0baacdf8c0419f14819b4.
>>    
>>    This problem is sidetracked by JDK-8265102.
>>  - fix runtime/logging/RedefineClasses.java crashed with -XX:+AsyncLogging
>>    
>>    nmethod::print(outputStream* st) should not obtain tty_lock by assuming
>>    st is defaultStream. It could be logStream as well.
>>    
>>    Currently, AyncLogFlusher::_lock has the same rank of tty_lock.
>>    https://issues.amazon.com/issues/JVM-563
>>  - ... and 8 more: https://git.openjdk.java.net/jdk/compare/0feed460...4cf4a57f
>
> src/hotspot/share/logging/logAsyncFlusher.hpp line 34:
> 
>> 32: 
>> 33: template <typename E>
>> 34: class LinkedListDeque : private LinkedListImpl<E, ResourceObj::C_HEAP, mtLogging> {
> 
> The name `LinkedListDeque` implies that this is a general purpose Deque implementation which is not true. It's fine to have an implementation for your very specific needs (otherwise it should probably be in its own file under `share/utilities/dequeue.hpp`). But to make this explicitly clear to the reader, can you please rename it to something like `AsyncFlusherDeque` and specify it's semantics in a comment on top of the class. E.g this class doesn't support the usage of the inherited `add()` method because that would break the `size()` functionality.

I have changed the class declaration to 
`template <typename E, MEMFLAGS F> class LinkedListDeque `

Per your request, I have moved  all logging logics out of it, so it can be used as a generic LinkedListDeque.  I think it's fair enough to keep its name.  In logging logics, I use a typedef version here: 
`typedef LinkedListDeque<AsyncLogMessage, mtLogging> AsyncLogBuffer;`

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

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