RFR: 8323807: Async UL: Add a stalling mode to async UL [v10]

[David Holmes]

On Wed, 22 Jan 2025 14:57:55 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> In January of this year I took a stab at implementing a stalling mode for UL, see link: https://github.com/openjdk/jdk/pull/17757 . I also talked about this feature in the mailing lists and seemed to receive positive feedback. With that PR, I also implemented a circular buffer. This PR didn't go through because 1. The stalling mode was broken 2. The complexity was a bit too large imho.
>> 
>> This PR does a much smaller change by only focusing on implementing the actual stalling.
>> 
>> The addition in terms of command line changes are the same as before, you can now specify the mode of your async logging:
>> 
>> 
>> $ java -Xlog:async:drop # Dropping mode, same as today
>> $ java -Xlog:async:stall # Stalling mode!
>> $ java -Xlog:async # Dropping mode by default still
>> 
>> 
>> The change in protocol is quite simple. If a producer thread `P` cannot fit a message into the buffer, it `malloc`s a message and exposes it via a shared pointer. It blocks all other producer threads from writing into the buffer. At the same time, the consumer thread (`AsyncLogWriter`) will perform all writing. When the consumer thread has emptied the write buffer, it writes the stalled message, notifies `P` and releases all locks. `P` then let's all other producer threads continue.
>> 
>> We do this by having two locks: `Outer` and `Inner`. In our example above, `P` prevents any other producers from progressing by holding the outer lock, but allows the consumer thread to progress by releasing the inner lock.
>> 
>> In pseudo-code we have something like this in the stalling case.
>> 
>> 
>> void produce() {
>>   OuterLock olock;
>>   InnerLock ilock;
>>   bool out_of_memory = attempt_produce(shared_buffer);
>>   if (out_of_memory) {
>>     pmsg = new Message();
>>     shared_message = pmsg;
>>     while (shared_message != nullptr) ilock.wait();
>>     free(pmsg);
>>   }
>> }
>> 
>> void consume() {
>>   InnerLock ilock;
>>   consume(shared_buffer);
>>   if (shared_message != nullptr) {
>>     consume(shared_message);
>>     ilock.notify();
>>   }
>> }
>> 
>> 
>> *Note!* It is very important that the consumer prints all output found in the buffer before printing the stalled message. This is because logging is output in Program Order. In other words: `print(m0); print(m1);` means that `m0` must appear before `m1` in the log file.
>> 
>> *Note!* Yes, we do force *all* threads to stall before the original stalled message has been printed. This isn't optimal, but I still have hope that we can switch to a faster circu...
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   NOT_DEBUG is not the same as PRODUCT_ONLY I assume

My understanding of this is now on very shakey ground with the new changes. So revoking my review.

src/hotspot/share/logging/logAsyncWriter.cpp line 187:

> 185:   if (req > 0) {
> 186:     assert(req == 1, "Only one token is allowed in queue. AsyncLogWriter::flush() is NOT MT-safe!");
> 187:     return true;

I'm rusty on the details here. What prevents the loop above from encountering more than one flush token?

Regardless why was it necessary to move the sem interaction into the caller?

src/hotspot/share/logging/logAsyncWriter.cpp line 309:

> 307:   AsyncLogWriter* instance = AsyncLogWriter::instance();
> 308:   if (instance != nullptr) {
> 309:     if ((uintptr_t)instance == (uintptr_t)Thread::current_or_null()) {

Why the casts ??

src/hotspot/share/logging/logAsyncWriter.cpp line 312:

> 310:       // If logging from the consuming thread then fall back on synchronous logging.
> 311:       // Otherwise, the consuming thread may wait on itself to print the message,
> 312:       // this obviously leads to a deadlocked system.

I'm somewhat surprised that we already allowing for logging within the logging thread as that potentially leads to infinite recursion. ??

src/hotspot/share/logging/logAsyncWriter.hpp line 221:

> 219:                                      const char* msg);
> 220:   static bool enqueue_if_initialized(LogFileStreamOutput& output,
> 221:                                      LogMessageBuffer::Iterator msg_iterator);

Are these really part of the public API? Couldn't the additional logic just be folded into the enqueue method?

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22770#pullrequestreview-2568780410
PR Review Comment: https://git.openjdk.org/jdk/pull/22770#discussion_r1926310085
PR Review Comment: https://git.openjdk.org/jdk/pull/22770#discussion_r1926316773
PR Review Comment: https://git.openjdk.org/jdk/pull/22770#discussion_r1926317921
PR Review Comment: https://git.openjdk.org/jdk/pull/22770#discussion_r1926318419