Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8229517: Support for optional asynchronous/buffered logging #3135

Closed
wants to merge 39 commits into from
Closed
Changes from 32 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
0697587
8229517: Support for optional asynchronous/buffered logging
Mar 22, 2021
a3945d0
8229517: Support for optional asynchronous/buffered logging
Mar 22, 2021
e23657f
8229517: Support for optional asynchronous/buffered logging
Mar 22, 2021
2871894
8229517: Support for optional asynchronous/buffered logging
Mar 22, 2021
760ec76
8229517: Support for optional asynchronous/buffered logging
Mar 28, 2021
bcefbec
8229517: Support for optional asynchronous/buffered logging
Mar 28, 2021
09c058d
8229517: Support for optional asynchronous/buffered logging
Apr 1, 2021
9211121
8229517: Support for optional asynchronous/buffered logging
Apr 9, 2021
81b2a0c
fix runtime/logging/RedefineClasses.java crashed with -XX:+AsyncLogging
Apr 9, 2021
54aadfa
Revert "fix runtime/logging/RedefineClasses.java crashed with -XX:+As…
Apr 12, 2021
f453d03
Inject the number of dropped messages since last dumpping.
Apr 15, 2021
3d74a32
Fix build issue with `--disable-precompiled-headers`
Apr 15, 2021
2bd104f
Support Jcmd pid VM.log disable
Apr 16, 2021
edb15c6
Remove LogAsyncInterval completely
Apr 20, 2021
11c2b7a
Resolve rank conflict between tty_lock and LogAsyncFlusher's _lock.
Apr 21, 2021
b2ad511
Fix a race condition bug on LogAsyncFlusher termination.
Apr 22, 2021
13c1c2b
Merge branch 'master' into JDK-8229517
Apr 22, 2021
4cf4a57
Refactor LogAsyncFlusher::abort()
Apr 23, 2021
1f06be3
Accurate Decorations for AsyncLogging.
Apr 23, 2021
e2d6f9e
Fix bugs/style/typo based on reviewers' feedbacks.
Apr 23, 2021
78d8f2c
Revert "Accurate Decorations for AsyncLogging."
Apr 29, 2021
e035ee3
Reimplement Accurate Decorations for AsyncLogging.
Apr 29, 2021
98ae33f
Implement the global option -Xlog:async per CSR.
May 3, 2021
1d5e540
Use LogTagSetMapping<LogTag::__NO_TAG>::tagset() instead of NULL poin…
May 5, 2021
c67d74e
Change option AsyncLogBufferEntries to AsyncLogBufferSize.
May 7, 2021
e3ee430
Merge branch 'master' into JDK-8229517
May 8, 2021
908d2a3
Update based on the feedbacks from reviwers.
May 11, 2021
7df8b1e
Fix build on Windows.
May 12, 2021
8f7b3b1
Refactor Initialize() and Termiante().
May 13, 2021
cade304
Replace Mutex with Semaphore.
May 14, 2021
e19e90a
refactor code and update per reviewers' feedback.
May 16, 2021
be83aaf
Implement the new discard policy: drop the incoming message.
May 17, 2021
348edfc
Update according to reviewer's feedback.
May 20, 2021
2381ba1
Add threadump support for AsyncLog Thread.
May 22, 2021
cad42d7
Update according to reviewers' feedback (Part-2).
May 22, 2021
9800a8c
flush() waits until all pending logging IO operations are done.
May 24, 2021
a25b8d6
Biased to the thread which is taking buffer lock.
May 24, 2021
2ffc33f
Remove AsyncLogWriter::flush() from os::shutdown().
May 25, 2021
b3d36cf
Correct typos and add a comment for a corner case.
May 27, 2021
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -29,6 +29,7 @@
#endif
#include "jvmtifiles/jvmti.h"
#include "logging/log.hpp"
#include "logging/logAsyncWriter.hpp"
#include "memory/allocation.inline.hpp"
#include "os_posix.inline.hpp"
#include "runtime/globals_extension.hpp"
@@ -1946,6 +1947,9 @@ void os::shutdown() {
// needs to remove object in file system
AttachListener::abort();

// flush buffered messages
AsyncLogWriter::flush();

// flush buffered output, finish log files
ostream_abort();

@@ -38,6 +38,7 @@
#include "jvmtifiles/jvmti.h"
#include "logging/log.hpp"
#include "logging/logStream.hpp"
#include "logging/logAsyncWriter.hpp"
#include "memory/allocation.inline.hpp"
#include "oops/oop.inline.hpp"
#include "os_share_windows.hpp"
@@ -714,6 +715,7 @@ bool os::create_thread(Thread* thread, ThreadType thr_type,
case os::vm_thread:
case os::pgc_thread:
case os::cgc_thread:
case os::asynclog_thread:
case os::watcher_thread:
if (VMThreadStackSize > 0) stack_size = (size_t)(VMThreadStackSize * K);
break;
@@ -1094,6 +1096,9 @@ void os::shutdown() {
// allow PerfMemory to attempt cleanup of any persistent resources
perfMemory_exit();

// flush buffered messages
AsyncLogWriter::flush();

// flush buffered output, finish log files
ostream_abort();

@@ -0,0 +1,199 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/
#include "precompiled.hpp"
#include "jvm.h"
#include "logging/logAsyncWriter.hpp"
#include "logging/logConfiguration.hpp"
#include "logging/logFileOutput.hpp"
#include "logging/logHandle.hpp"
#include "runtime/atomic.hpp"

Semaphore AsyncLogWriter::_sem(0);

class AsyncLogLocker : public StackObj {
private:
debug_only(static intx _locking_thread_id;)
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@albertnetymk

albertnetymk May 22, 2021
Member Outdated

It's unclear to me what this variable is for.

This comment has been minimized.

@navyxliu

navyxliu May 24, 2021
Author Member Outdated

thanks. I will delete it.

static Semaphore _lock;
public:
AsyncLogLocker() {
_lock.wait();
debug_only(_locking_thread_id = os::current_thread_id());
}

~AsyncLogLocker() {
debug_only(_locking_thread_id = -1);
_lock.signal();
}
};

Semaphore AsyncLogLocker::_lock(1);
debug_only(intx AsyncLogLocker::_locking_thread_id = -1;)

void AsyncLogWriter::enqueue_locked(const AsyncLogMessage& msg) {
if (_buffer.size() >= _buffer_max_size) {
const AsyncLogMessage* h = _buffer.front();
assert(h != NULL, "sanity check");

if (h->message() != nullptr) {
bool p_created;
uint32_t* counter = _stats.add_if_absent(h->output(), 0, &p_created);
*counter = *counter + 1;
}
// drop the enqueueing message.
return;
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member

I don't understand this? If we drop the incoming message msg, why do we increment the drop counter for the LogFileOutput of the first (i.e. front()) enqueued message?
Shouldn't this simply be:

if (_buffer.size() >= _buffer_max_size)  {
  bool p_created;
  uint32_t* counter = _stats.add_if_absent(msg->output(), 0, &p_created);
  *counter = *counter + 1;
  return;

This comment has been minimized.

@navyxliu

navyxliu May 19, 2021
Author Member

you are right. I will change this.
in previous implementation, I pop_front() and enqueue msg.
I didn't clean up everything.

}

assert(_buffer.size() < _buffer_max_size, "_buffer is over-sized.");
_buffer.push_back(msg);
_sem.signal();
}

void AsyncLogWriter::enqueue(LogFileOutput& output, const LogDecorations& decorations, const char* msg) {
AsyncLogMessage m(output, decorations, os::strdup(msg));

{ // critical area
AsyncLogLocker lock;
enqueue_locked(m);
}
}

// LogMessageBuffer consists of a multiple-part/multiple-line messsages.

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

typo: messages -> message

// the mutex here gurantees its integrity.

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

typo: gurantees -> guarantees

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

Reminder for myself: should replace Mutex with lock. Implementation has changed.

void AsyncLogWriter::enqueue(LogFileOutput& output, LogMessageBuffer::Iterator msg_iterator) {
AsyncLogLocker lock;

for (; !msg_iterator.is_at_end(); msg_iterator++) {
AsyncLogMessage m(output, msg_iterator.decorations(), os::strdup(msg_iterator.message()));
enqueue_locked(m);
}
}

AsyncLogWriter::AsyncLogWriter()
: _state(ThreadState::NotReady),
_stats(17 /*table_size*/) {
if (os::create_thread(this, os::asynclog_thread)) {
_state = ThreadState::Initialized;
}

log_info(logging)("The maximum entries of AsyncLogBuffer: " SIZE_FORMAT ", estimated memory use: " SIZE_FORMAT " bytes",
_buffer_max_size, AsyncLogBufferSize);
}

class AsyncLogMapIterator {
AsyncLogBuffer& _logs;

public:
AsyncLogMapIterator(AsyncLogBuffer& logs) :_logs(logs) {}
bool do_entry(LogFileOutput* output, uint32_t* counter) {
using none = LogTagSetMapping<LogTag::__NO_TAG>;

if (*counter > 0) {
LogDecorations decorations(LogLevel::Warning, none::tagset(), output->decorators());
stringStream ss;
ss.print(UINT32_FORMAT_W(6) " messages dropped due to async logging", *counter);
AsyncLogMessage msg(*output, decorations, ss.as_string(true /*c_heap*/));
_logs.push_back(msg);
*counter = 0;
}

return true;
}
};

void AsyncLogWriter::perform_IO() {

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

Seems to be all O and no I :)

This comment has been minimized.

@navyxliu

navyxliu May 22, 2021
Author Member Outdated

Let's call it "IO" for tradition. It would become more confusing if I changed it to perform_O

This comment has been minimized.

@navyxliu

navyxliu May 22, 2021
Author Member Outdated

okay. change it to 'write()'

// use kind of copy-and-swap idiom here.
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Can you please start new sentences with an uppercase letter.

This comment has been minimized.

@navyxliu

navyxliu May 19, 2021
Author Member Outdated

Okay.

// Empty 'logs' 'swaps' the content with _buffer.
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Only quote variables, i.e.: Empty 'logs' swaps the content with '_buffer'.

// Along with logs destruction, all procceeded messages are deleted.

This comment has been minimized.

@phohensee

phohensee May 20, 2021
Member Outdated

procceeded -> preceding

This comment has been minimized.

@navyxliu

navyxliu May 20, 2021
Author Member Outdated

typo: proccessed

//
// the atomic operation 'move' is done in O(1). All I/O jobs are done without lock.
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Start with uppercase letter. Replace 'move' by 'pop_all()' because at this point it in´s not obvious that 'pop_all()' eventually calls 'LinkedList::move()'.
What do you mean with "atomic" here? Maybe better remove?

This comment has been minimized.

@navyxliu

navyxliu May 20, 2021
Author Member Outdated

_buffer.pop_all(&logs) is placed in 'critical region' on purpose.
It's because logsites may be enqueuing concurrently. that's why I need to use a lock to protect _buffer.
I see 'atomic' is inappropriate in the context. I will remove it.

// This guarantees I/O jobs don't block logsites.
AsyncLogBuffer logs;
{ // critical region
AsyncLogLocker ml;
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@albertnetymk

albertnetymk May 22, 2021
Member Outdated

For consistency, I suggest lock as the name.

AsyncLogMapIterator dropped_counters_iter(logs);

_buffer.pop_all(&logs);
// append meta-message of dropped counters
_stats.iterate(&dropped_counters_iter);
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@albertnetymk

albertnetymk May 22, 2021
Member

Any particular reason why the iterator is not declared right above its usage? IOW, why not

_buffer.pop_all(&logs);
AsyncLogMapIterator dropped_counters_iter(logs);
_stats.iterate(&dropped_counters_iter);

This comment has been minimized.

@navyxliu

navyxliu May 24, 2021
Author Member

yes, it works. I will update it.
I unconsciously wrote code in C style. ie. declarations come first, code snippet follows.

}

LinkedListIterator<AsyncLogMessage> it(logs.head());
while (!it.is_empty()) {
AsyncLogMessage* e = it.next();
char* msg = e->message();

if (msg != nullptr) {
e->output()->write_blocking(e->decorations(), msg);
os::free(msg);
}
}
}

void AsyncLogWriter::run() {
assert(_state == ThreadState::Running, "sanity check");

while (_state == ThreadState::Running) {
_sem.wait();
perform_IO();
}

assert(_state == ThreadState::Terminated, "sanity check");

This comment has been minimized.

@albertnetymk

albertnetymk May 21, 2021
Member Outdated

I can't seem to find where _state is set to ThreadState::Terminated in this PR.

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

Yes, it's another historical problem. I used to have AsyncLogWriter::terminate(), but Thomas said I can avoid the complexity by leaving it alone.

remove if not in use?

This comment has been minimized.

@albertnetymk

albertnetymk May 21, 2021
Member Outdated

remove if not in use?

Yes, please.

Shouldn't this assertion be triggered as it is now? Why no failing tests?

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

it's because while (_state == ThreadState::Running) is a dead loop.
Nobody attempts to terminate AsyncLog Thread.

perform_IO(); // in case there are some messages left
}

AsyncLogWriter* AsyncLogWriter::_instance = nullptr;

void AsyncLogWriter::initialize() {
if (!LogConfiguration::is_async_mode()) return;

if (_instance == nullptr) {

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

This method should only be called once so _instance must be NULL. An assert rather than an if would suffice.

AsyncLogWriter* self = new AsyncLogWriter();

if (self->_state == ThreadState::Initialized) {

This comment has been minimized.

@albertnetymk

albertnetymk May 21, 2021
Member Outdated

The content of this if could be inlined to the constructor, and ThreadState::Initialized could be removed then, right?

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

but it's not exceptional safe. I know hotspot doesn't use c++ exception, but the general principle still applys.
the more logics in ctor, the more likely to fail. So far, if something wrong, I left self in state of NotReady.

This comment has been minimized.

@albertnetymk

albertnetymk May 21, 2021
Member Outdated

Since hotspot does not use c++ exception, I don't really follow the reasoning of "the more logics in ctor, the more likely to fail". Code doesn't just fail mysteriously. The only thing that could "fail" here is new AsyncLogWriter() returns nullptr, I think.

Anyway, this is just a suggestion; feel free to go for the style you prefer.

Atomic::release_store_fence(&AsyncLogWriter::_instance, self);
// all readers of _instance after fence see NULL,

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Please start sentences with uppercase letter.

I don't understand this? Wouldn't all readers of '_instance' see 'self' after it was stored there with 'Atomic::release_store_fence()`? Or do you wanted to say before the fence here?

This comment has been minimized.

@navyxliu

navyxliu May 20, 2021
Author Member Outdated

My fault. I mean "All readers of _instance after the fence see non-NULL".

// but we still need to ensure no active reader of any tagset.
// After then, we start AsyncLog Thread and it exclusively takes
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

After that...

// over all logging I/O.
for (LogTagSet* ts = LogTagSet::first(); ts != NULL; ts = ts->next()) {
ts->wait_until_no_readers();
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

This loop is the only thing I'm not understanding in this whole PR but that doesn't mean it is wrong (it's probably just caused by my limited understanding of UL).
So why do we have to wait here until "there are no more readers" and what does that actually means? Is this necessary to prevent mixing log messages which have been printed before async logging was started?

This comment has been minimized.

@navyxliu

navyxliu May 19, 2021
Author Member Outdated

I would like to consolidate "exclusively". Once async logging takes over, we guarantee that no synchronous logsite is writing files in concurrency. If we guarantee that, we can avoid FileLocker() because only AsyncLog Thread writes files.

Fence isn't enough here. fence can only guarantee that logsites 'after' the fence use async logging. It's possible that there are on-going synchronous logging.

This is a RCU trick of Unified logging. RCU is kind of synchronization biased to multiple readers. LogOutputList::Iterator actually embeds an atomic counter.
ts->wait_until_no_readers() waits until all readers finish. It means that all synchronous write have done.

here is how synchronous log writes. Iterator increases to zero when the loop is done.

void LogTagSet::log(LogLevelType level, const char* msg) {
  LogDecorations decorations(level, *this, _decorators);
  for (LogOutputList::Iterator it = _output_list.iterator(level); it != _output_list.end(); it++) {
    (*it)->write(decorations, msg);
  }
}```

This comment has been minimized.

@simonis

simonis May 20, 2021
Member Outdated

Thanks for the explanation. I assume you meant that the Iterator decreases the readers to zero when the loop is done. But I think I got it now.

This comment has been minimized.

@navyxliu

navyxliu May 20, 2021
Author Member Outdated

Yes, the scope of 'it' is defined inside the 'for' construct. Once control flow exists the loop,
the dtor of Iterator will automatically be invoked. When all pending synchronous logsites have completed, all RCU counters of OutputList objects restore to zero. It means there's no on-going 'reader'.

    ~Iterator() {
      _list->decrease_readers();
    }
}
self->_state = ThreadState::Running;

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

This seems like something the thread itself should do at the start of run()

os::start_thread(self);
log_debug(logging, thread)("AsyncLogging starts working.");

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

Suggest: "Async logging thread started"

} else {
log_warning(logging, thread)("AsyncLogging failed to launch thread. fall back to synchronous logging.");

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

"Async logging failed to create thread. Falling back to synchronous logging."

Shouldn't this be done where the os::create_thread failed though?

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

make sense. I think I can move "Async logging failed to create thread. Falling back to synchronous logging."

}
}
}

AsyncLogWriter* AsyncLogWriter::instance() {
return _instance;
}

void AsyncLogWriter::flush() {
if (_instance != nullptr) {
_instance->perform_IO();
}
}
@@ -0,0 +1,154 @@
/*
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/
#ifndef SHARE_LOG_ASYNC_WTRITER_HPP
#define SHARE_LOG_ASYNC_WTRITER_HPP
Comment on lines +24 to +25

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

Typos: WTRITER

This comment has been minimized.

@navyxliu

navyxliu May 22, 2021
Author Member Outdated

ack

#include "logging/log.hpp"
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@phohensee

phohensee May 20, 2021
Member

Use SHARE_LOGGING_LOGASYNCHWRITER_HPP to conform with existing naming standard.

This comment has been minimized.

@navyxliu

navyxliu May 22, 2021
Author Member

Oh, there's a hidden naming convention here.
thanks for the head-up!

#include "logging/logDecorations.hpp"
#include "logging/logFileOutput.hpp"
#include "logging/logMessageBuffer.hpp"
#include "memory/resourceArea.hpp"
#include "runtime/nonJavaThread.hpp"
#include "utilities/hashtable.hpp"
#include "utilities/linkedlist.hpp"

This conversation was marked as resolved by navyxliu

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member

Could you write a summary of operation comment explaining how async mode works e.g. details on initialization and teardown, how the thread interacts with writers etc. Thanks.

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member

How about this?

initialize() is called once when JVM is initialized. It creates and initialize the singleton instance of AsyncLogWriter. Once async logging is established, there's no way to turn it off.

instance() is MT-safe and returns the pointer of the singleton instance if and only if async mode is on and has well initialized. Clients can use its return value to determine async logging is established or not.

The basic operation of AsyncLogWriter is enqueue. Two overloading versions of it are provided to match LogOutput::write. To support async logging, derived classes of LogOutput can invoke the corresponding AsyncLogWriter::enqueue in write() and return -1. AsyncLogWriter is responsible of copying both decorations and cstr messages and managing their lifecycle.

The static member function flushis designated to flush out all pending messages when JVM is terminating or aborting. In normal JVM termination, flush() is invoked in LogConfiguration::finalize(). In abortion situation, flush() is invoked in os::shutdown(). flush is MT-safe and can be invoked arbitrary times. It is no-op if asynclogging is not established.

template <typename E, MEMFLAGS F>
class LinkedListDeque : private LinkedListImpl<E, ResourceObj::C_HEAP, F> {
private:
LinkedListNode<E>* _tail;
size_t _size;

public:
LinkedListDeque() : _tail(NULL), _size(0) {}
void push_back(const E& e) {
if (!_tail) {
_tail = this->add(e);
} else {
_tail = this->insert_after(e, _tail);
}

++_size;
}

// pop all elements to logs.
void pop_all(LinkedList<E>* logs) {
logs->move(static_cast<LinkedList<E>* >(this));
_tail = NULL;
_size = 0;
}

void pop_all(LinkedListDeque<E, F>* logs) {
logs->_size = _size;
logs->_tail = _tail;
pop_all(static_cast<LinkedList<E>* >(logs));
}

void pop_front() {
LinkedListNode<E>* h = this->unlink_head();
if (h == _tail) {
_tail = NULL;
}

if (h != NULL) {
--_size;
this->delete_node(h);
}
}

size_t size() const { return _size; }

const E* front() const {
return this->_head == NULL ? NULL : this->_head->peek();
}

const E* back() const {
return _tail == NULL ? NULL : _tail->peek();
}

LinkedListNode<E>* head() const {
return static_cast<const LinkedList<E>* >(this)->head();
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Why not simply return this->_head?

This comment has been minimized.

@navyxliu

navyxliu May 20, 2021
Author Member Outdated

yes, it works!

}
};

class AsyncLogMessage {
LogFileOutput& _output;
const LogDecorations _decorations;
char* _message;

public:
AsyncLogMessage(LogFileOutput& output, const LogDecorations& decorations, char* msg)
: _output(output), _decorations(decorations), _message(msg) {}

// placeholder for LinkedListImpl.
bool equals(const AsyncLogMessage& o) const { return false; }

LogFileOutput* output() const { return &_output; }
const LogDecorations& decorations() const { return _decorations; }
char* message() const { return _message; }
};

typedef LinkedListDeque<AsyncLogMessage, mtLogging> AsyncLogBuffer;
typedef KVHashtable<LogFileOutput*, uint32_t, mtLogging> AsyncLogMap;

class AsyncLogWriter: public NonJavaThread {

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

Any reason this is not a NamedThread? How will this appear in thread-dumps and crash reports?

Also I would have expected two classes to simplify the APIs: AsyncLogWriter and AsyncLogThread

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

NamedThread() is not just NonJavaThread with a name. It has some gc-specific fields. it seams NamedThread is for GC threads. Further, I don't need to support dynamic names.

I just figure out Threads::print_on() needs special care for dumpthread. Nice catch!
With my patch, it finally shows up.

"AsyncLog Thread" os_prio=0 cpu=239.26ms elapsed=52.13s tid=0x00007fb814c76ec0 nid=0x9023 runnable

Now there are only 4 APIs for clients. Splitting up 2 classes seems not to simplify code a lot.

private:
static AsyncLogWriter* _instance;
static Semaphore _sem;
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member

What is this semaphore for? A comment and/or a more meaningful name would be good.

This comment has been minimized.

@navyxliu

navyxliu May 22, 2021
Author Member

I added some comments here and _sem.wait().


enum class ThreadState {
NotReady,
Initialized,
Running,
Terminated,
This conversation was marked as resolved by navyxliu
Comment on lines +121 to +122

This comment has been minimized.

@albertnetymk

albertnetymk May 24, 2021
Member Outdated

These two states can be removed now, right? If so, maybe having a binary state (volatile bool _initialized) is enough and easier to follow.

This comment has been minimized.

@navyxliu

navyxliu May 24, 2021
Author Member Outdated

make sense. I will update it along with the trywait() patch.

};

volatile ThreadState _state;
AsyncLogMap _stats; // statistics of dropping messages.

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

// statistics for dropped messages

AsyncLogBuffer _buffer;

// The memory use of each AsyncLogMessage(payload) consist of itself, a logDecoration object
// and a variable-length c-string message.
// A normal logging message is smaller than vwrite_buffer_size, which is defined in logtagset.cpp
const size_t _buffer_max_size = {AsyncLogBufferSize / (sizeof(AsyncLogMessage) + sizeof(LogDecorations) + vwrite_buffer_size)};
This conversation was marked as resolved by navyxliu

This comment has been minimized.

@simonis

simonis May 19, 2021
Member Outdated

Isn't the LogDecorations object part of the AsyncLogMessage object? It is stored there as a "value object" not as a reference so its size should already be included in sizeof(AsyncLogMessage).

This comment has been minimized.

@navyxliu

navyxliu May 19, 2021
Author Member Outdated

yes, I changed logDecoration from pointer to value. I will update the comment.


AsyncLogWriter();
void enqueue_locked(const AsyncLogMessage& msg);
void perform_IO();
void run() override;
void pre_run() override {
NonJavaThread::pre_run();
log_debug(logging, thread)("starting AsyncLog Thread tid = " INTX_FORMAT, os::current_thread_id());
}
char* name() const override { return (char*)"AsyncLog Thread"; }

public:
void enqueue(LogFileOutput& output, const LogDecorations& decorations, const char* msg);
void enqueue(LogFileOutput& output, LogMessageBuffer::Iterator msg_iterator);

// None of following functions are thread-safe.
static AsyncLogWriter* instance();
Comment on lines +148 to +149

This comment has been minimized.

@dholmes-ora

dholmes-ora May 21, 2021
Member Outdated

instance() isn't thread-safe ??

This comment has been minimized.

@navyxliu

navyxliu May 21, 2021
Author Member Outdated

yes, the comment is outdated. This is MT-safe.
I will update it.

static void initialize();
static void flush();
};

#endif // SHARE_LOG_ASYNC_WTRITER_HPP