RFR: 8292989: Avoid dynamic memory in AsyncLogWriter

[Johan Sjölén]

On Wed, 31 Aug 2022 01:10:09 GMT, Xin Liu <xliu at openjdk.org> wrote:

> Current implementation of AsyncLogWriter uses dynamic memory. There are t2 sources. 
> 
> 1. Overhead of pointer-based linked-list. 
> 2. strdup of message contents
> 
> This implementation has impact on glibc/malloc. If allocation of logsites interleave with other allocation, it's hard to clean up all glibc arenas. This worsens fragmentation issue.
> 
> In this PR, we replace linked-list with `2 pre-allocated raw buffers`. AsyncLogWriter appends payload AsyncLogMessage to the serving buffer and avoids all dynamic allocation. Please note this effort won't eliminate mutex lock. We use ping-pong buffers to guarantee AsyncLogWriter is still non-blocking. A buffer serves as a FIFO queue like before. 
> 
> In addition, AsyncLogWriter doesn't enqueue meta messages anymore when it needs to report the number of discarded messages. This is archived using a temporary hashtable called `snapshot`. It copies the working hashtable with the protection of lock and reset it. After writing all regular messages, AsyncLogWriter writes meta messages from snapshot.

Thank you for your PR. A few comments from me.

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

> 79: const LogDecorations& AsyncLogWriter::None = LogDecorations(LogLevel::Warning, LogTagSetMapping<LogTag::__NO_TAG>::tagset(),
> 80:                                       LogDecorators::None);
> 81: // Note: there is an initializer order issue here. C++ cannot guarantee that 'None' is initialized before Token!

Can we avoid initializing these like this then? What happens if our expectations are not fulfilled some day in the future? I've been bit by bugs caused by this before, not very fun. Is there a reason they can't be initialized in AsyncLogWriter::initialize() for example?

Context for the curious:

https://en.cppreference.com/w/cpp/language/initialization

https://isocpp.org/wiki/faq/ctors#static-init-order

To be pedantic: I believe that None is guaranteed to be initialized before Token, but the LogTagSet which None is assigned to is not guaranteed to be initialized (because they're part of two different compilation units).

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

> 123:   size_t size = align_up(AsyncLogBufferSize / 2, page_size);
> 124: 
> 125:   char* buf0 = static_cast<char* >(os::malloc(size, mtLogging));

Is there a particular reason that AsyncLogWriter allocates and handles the resources intended for `Buffer` instead of `Buffer` doing it itself in its ctr and dtr?

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

> 141:     log_warning(logging)("AsyncLogging failed to create buffers. Falling back to synchronous logging.");
> 142: 
> 143:     if (buf0 != nullptr) {

`os::free(nullptr)` is safe, no need for a branch here.

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

> 96:     bool push_back(const AsyncLogMessage& msg);
> 97: 
> 98:     // for testing-only!

We typically allow test fixture classes to be declared `friend`s to the classes they test. This allows you to move testing functions to the testing code.

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

PR: https://git.openjdk.org/jdk/pull/10092