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

[Thomas Stuefe]

On Fri, 9 Sep 2022 22:23:05 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> 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?
>
> Currently, there's only one token, ie. the flushing token. We use it to implement "flushing" semantic. 
> 
> `AsyncLogWriter::flush` inserts it and is blocked until "flushing" is complete. 
> Therefore, one queue can only have one token anytime. I reserve it in ctor so enqueuing of the token always succeeds. Unlike regular enqueuing, failure is not acceptable in this case because it will hang AsyncLogWriter thread. 
> 
> Previously, I have an object called 'Token'. Since now the ctor only works for in-place new, I can't construct it on c-heap or stack anymore. I use a tuple {nullptr,  AsyncLogWriter::None, ""} instead. here is function. 
> 
> void AsyncLogWriter::Buffer::push_flush_token() {
>   bool result = push_back(nullptr, AsyncLogWriter::None, "");
>   assert(result, "fail to enqueue the flush token");
> }
> 
> 
> It's impossible to enqueue a non-token object from outside because the public interface `AsyncLogWriter::enqueue` only receives LogFileStreamOutput& as the 1st argument. There's no way to pass in null reference in C++.

Ah, I see. This is easy to miss. May I suggest a slight alteration that makes it easier to understand to the casual reader?

In Message, define a static function that calculates message byte size (the various definitions existing make me nervous, easy for them to drift apart):

    // Calculate size for a prospective Message object depending on its message length
    static constexpr size_t calc_size(int message_len) {
      return align_up(sizeof(Message) + message_len + 1, sizeof(void*));
    }


Use this in Message::size().

Get rid of TOKEN, and in the ctor, let _capacity be the real capacity. Instead, in push_back, do:


bool AsyncLogWriter::Buffer::push_back(LogFileStreamOutput* output, const LogDecorations& decorations, const char* msg) {
  const size_t sz = Message::calc_size(strlen(msg));
  const bool is_push_token = output == nullptr;
  // Always leave headroom for the push token. Pushing the push token must always succeed.
  const size_t headroom = (!is_push_token) ? Message::calc_size(0) : 0;
  if (_pos + sz <= (_capacity - headroom)) {
    new(_buf + _pos) Message(output, decorations, msg);
    _pos += sz;
    return true;
  }
  return false;
}


Now the magic is in exactly one place, and easy to understand with one glance. And we have one central place to calculate the message byte size.

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

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