[jdk17] RFR: 8267952: async logging supports to dynamically change tags and decorators

[David Holmes]

On 24/06/2021 2:33 pm, Xin Liu wrote:
> On Wed, 23 Jun 2021 20:18:32 GMT, Xin Liu <xliu at openjdk.org> wrote:
> 
>> This patch fixed intermittent crashes of gtest/AsyncLogGtest.java. The root cause
>> is that previous flush() can't guarantee flush all pending messages in AsyncLog
>> buffer. This patch implements flush() using a synchronizaton token and waits for
>> completion. This approach isn't MT-safe but it can serialize all flush() calls in
>> a thread.
>>
>> Two concurrent tests are appended to cover the hazard cases.
>> This patch also comments LogConfiguration::configure_output() MT-safety.
> 
> hi, David,
>> I have one remaining concern, which is just something we need to keep an eye on, and that is the potential for delaying VM termination if the async log writer gets blocked/delayed.
> 
> We discussed this before. Yes, if "write_blocking()" does block on I/O part, flush() is blocked.  We have this issue before, and synchronous logging has this issue as well.    I think Async logging is better than sync logging on this because at least JVM got stuck at termination instead of any safepoint if it did happen.
> 
> It isn't perfect. Ideally, we can use flush_sem.wait(timeout) if Semaphore::wait supports timeout for all platforms.

I agree existing synchronous logging also has a (worse?) problem here. 
Not sure there is a suitable fix as any timeout will race with the 
asyncLog thread again potentially causing the crashes you just fixed.

>> I'm curious now about the fact the buffer size seems only a logical size constraint not an actual out-of-space constraint.
> 
> We can have an assertion like this. It's because flush() must be thread-safe. The reason I don't enforce it using mutex or yet another semaphore is that current use cases have already been protected. If all flush() are serialized, it means that there's one or none token at any time. therefore, the following assertion is right. Shall I put it back?
> 
> `
> assert(_buffer.size() < _buffer_max_size+1, "_buffer is over-sized.");
> `

Not sure where this assertion was.

But that wasn't what I was asking about. I thought we had 
AsyncLogBufferSize because we had a fixed size buffer and hence if the 
buffer were full you couldn't insert the token. BUt I was misremembering 
- the buffer size is not fixed (it is just a linked list) it is the 
number of buffer entries that is limited by AsyncLogBufferSize, to 
ensure we don't consume too much memory. So all is fine here.

>>   Warning: asynclog is OFF.
> It is from gtest fixture AsyncLogTest, which is expected. We run those tests in two modes.
> Normally, gtest run this test in synchronous mode. It doesn't test much.
> 
> https://github.com/openjdk/jdk/blob/master/test/hotspot/gtest/logging/test_asynclog.cpp#L37

Doh! I grepped in hotspot/jtreg by mistake. Thanks for clarifying.

Cheers,
David
-----

> Thank you for you patience. TBH, I coundn't wrap around my head if you did tell me that handshake problem.
> I will post a new revision to include your feedbacks.
> 
> thanks,
> --lx
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk17/pull/130
>