RFR: 8292989: Avoid dynamic memory in AsyncLogWriter [v4]

[Thomas Stuefe]

On Thu, 8 Sep 2022 06:24:48 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> Current implementation of AsyncLogWriter uses dynamic memory. There are 2 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.
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix MSVC warning C4267
>   
>   MSVC reports logAsyncWriter.cpp(62): warning C4267: 'initializing': conversion from
>   'size_t' to 'int', possible loss of data

Hi Xin,

getting closer. Remarks inline.

Cheers, Thomas

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

> 62:   size_t sz = align_up(sizeof(Message) + len, sizeof(void*));
> 63: 
> 64:   if (_pos + sz <= _capacity || output == nullptr/*token*/) {

I don't understand this. The push token thing is not zero-sized, so what happens if we go over capacity? Or, in other words, if we fill the buffer with push tokens only, won't we overwrite and crash?

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

> 78: AsyncLogWriter::Message* AsyncLogWriter::Buffer::Iterator::next() {
> 79:   assert(_curr < _buf._pos, "sanity check");
> 80:   auto msg = reinterpret_cast<Message*>(_buf._buf + _curr);

Is there a reason why Iterator needs to directly access _buf? Why not give Buffer a `const char* base() const` ? Also, why return a non-const ptr?

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

> 87:   // To save space and streamline execution, we just ignore null message.
> 88:   // client should use "" instead.
> 89:   if (msg == nullptr) return;

Is that even valid? Who logs with a NULL message? If that is valid, why treat it differently than "", and if not, why not just assert here?

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

> 121: 
> 122:   size_t page_size = os::vm_page_size();
> 123:   size_t size = align_up(AsyncLogBufferSize / 2, page_size);

Why page size alignment? Would also interfere with my proposal of testing with a tiny buffer.

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

> 63:                           ResourceObj::C_HEAP,
> 64:                           mtLogging>;
> 65:   class Message {

Maybe add a small comment about the memory layout. ("variable sized structure with zero-terminated string following the header").

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

> 95:     size_t _capacity;
> 96: 
> 97:    public:

Make NONCOPYABLE?

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

> 97:    public:
> 98:     Buffer(size_t capacity);
> 99:     ~Buffer();

I'd remove this destructor, or make it private, or at least add a ShouldNotReachHere. IIUC we never free the buffer, and that is by design.

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

> 107:       _capacity = value;
> 108:       return old;
> 109:     }

Actually, I would prefer it if you did not shrink the buffer dynamically, since this is not a supported scenario and may cause unforeseen problems (besides making the code harder to argue about). 

You could just leave buffer size immutable after creation and instead add another test variation to your `AsyncLogGtest.java` where you start with a tiny tiny buffer size, and then run all your gtests with it. More test coverage, and we don't need the test coding here.

(Side note, I see that in AsyncLogGtest.java you run all tests inside a single `@test` scope. If you split them up and give them speaking id's, these tests can run in parallel. For examples, see e.g. the NMTGtests. The duplication of the test section is annoying, but the parallelization of tests you get for free is cool. Also, you can start them individually if you want to reproduce a test error.)

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

> 119: 
> 120:       bool is_empty() const {
> 121:         return _curr >= _buf._pos;

This should never be larger than `_buf._pos`, right? Also, I would not name it `is_empty`, that's confusing, but rather something like `eof()` or `at_end()` or somesuch.

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

> 122:       }
> 123: 
> 124:       Message* next();

Why non-const?

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

Changes requested by stuefe (Reviewer).

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