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

[David Holmes]

Hi Volker,

On 26/05/2021 8:45 pm, Volker Simonis wrote:
> On Tue, 25 May 2021 05:23:56 GMT, Xin Liu <xliu at openjdk.org> wrote:
> 
>>> This patch provides a buffer to store asynchrounous messages and flush them to
>>> underlying files periodically.
>>
>> Xin Liu has updated the pull request incrementally with two additional commits since the last revision:
>>
>>   - Remove AsyncLogWriter::flush() from os::shutdown().
>>     
>>     When jvm aborts, it won't invoke AsyncLogWriter::flush() and guarantee
>>     not to hinder the aborting process.
>>     
>>     Run all unified logging tests in async mode.
>>   - Biased to the thread which is taking buffer lock.
> 
> Hi David,
> 
> is my understanding right that you're now fine with this change except for the question about potentially loosing logs in the case of an abort?

I was concerned about making the thread wait() when trying to abort, but 
Xin has changed that now as he now agrees there is an issue there. So 
that does mean we might lose some log entries on abort but I hadn't 
flagged that as an issue (but did someone else earlier?).

I was also checking on the exact role of the iosem and why the tests had 
failed - see my email earlier today on that. I'm not sure it is the best 
way to solve the problem but I'm okay with it.

> You have rightly argued that we don't want to wait an undefined amount of  time while we're in the process of aborting. As a consequence, Xin has removed the call to `AsyncLogWriter::flush()` from `os::shutdown()`. I think that's a good compromise because aborting is a rare event and even if aborting the probability of loosing log messages is very low. Using async logging already comes with the risk of loosing log messages in the case where the reserved buffer for async logging is too small. That's the tradeoff all users of async logging will have to pay for better latency. I don't think that the ultra-low probability of loosing logs in the case of a crash makes any difference for the general usability of this feature.
> 
> You've specifically asked for Thomas' opinion on this topic but he's out of the office for the next two weeks and I don't think that his absence should further block the integration of this patch. We've discussed this PR for more than two month in more than 250 messages. The PR has 4 approvals by now and I think it is really time to finally ship it.

Except that those approvals are not for the current updated version of 
the code, so I would expect to see re-reviews by as many of those who 
already reviewed it as possible. I really don't like it when we have a 
non-trivial feature like this and we don't get a consensus amongst 
reviewers on the final product. It is unfortunate that Thomas is away, 
as I would hate for him to come back and find this had gone in 
unexpected directions. I would really like to hear that the other 
reviewers are okay with how this code ended up.

Thanks,
David
-----

> Please let us know if you have any additional, sever objections? Otherwise it would be great if you could approve it as well.


> Thank you and best regards,
> Volker
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3135
>