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

[David Holmes]

On 24/05/2021 12:04 pm, Xin Liu wrote:
> On Sat, 22 May 2021 21:35:50 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:
> 
>>> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Update according to reviewers' feedback (Part-2).
>>
>> src/hotspot/share/logging/logAsyncWriter.hpp line 137:
>>
>>> 135: class AsyncLogWriter : public NonJavaThread {
>>> 136:   static AsyncLogWriter* _instance;
>>> 137:   // _sem is a semaphore whose value denotes how many messages have been enqueued.
>>
>> "how many messages have been enqueued" seems to suggest it's a monotonically growing number as new message are enqueued. In fact, this number will decrease as `_buffer` is flushed. Maybe "_sem denotes the number of messages in _buffer` and moving the two together?
> 
> The value of _sem denotes the number of messages have been enqueued.  It's actually different from "the number of messages in _buffer".  When  _sem.wait() returns, it just increases one.  however, _buffer.pop_all() pops all messages in queue at once.   The two numbers are not consistent all the time.

I'm finding the use of the semaphores quite confusing, so I will try to 
summarise things as I see them:

The _lock semaphore is used by AsyncLogLocker to serialize access to the 
intermediate buffer by the threads doing the logging, and to also 
coordinate with the log writer thread clearing the buffer. This is a 
semaphore because a Mutex can't be used when some of the logging takes 
places.

The _sem semaphore is signaled whenever a log item is enqueued, causing 
the log writer thread to unlock from wait() and process the current 
contents of the intermediate buffer. Because we enqueue one at a time 
but dequeue in bulk this means that the log writer thread will spin in:

  while (true) {
   _sem.wait();
   write();
  }

until _sem has a count of zero and so wait() will block. This seems 
inefficient but I'm not sure how to avoid that.

The _io_sem semaphore has just been introduced into 
AsyncLogWriter::write but I can't tell what it is actually trying to 
coordinate. ??

Thanks,
David
-----


>> Maybe "_sem denotes the number of messages in _buffer` and moving the two together?
> 
> Could you elaborate this?  thanks?
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3135
>