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

Conversation

@navyxliu
Copy link
Contributor

@navyxliu navyxliu commented Mar 22, 2021

This patch provides a buffer to store asynchrounous messages and flush them to
underlying files periodically.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8229517: Support for optional asynchronous/buffered logging

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3135/head:pull/3135
$ git checkout pull/3135

Update a local copy of the PR:
$ git checkout pull/3135
$ git pull https://git.openjdk.java.net/jdk pull/3135/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3135

View PR using the GUI difftool:
$ git pr show -t 3135

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3135.diff

Xin Liu added 4 commits Mar 22, 2021
Add an option async for the file-based outputs. The option conforms to
output-option in JEP-158.
fix a warning from c++11: "The generation of the implicitly-defined copy
assignment operator is deprecated if T has a user-declared destructor or
user-declared copy constructor."
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 22, 2021

👋 Welcome back xliu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 22, 2021

@navyxliu The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the hotspot label Mar 22, 2021
@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Mar 23, 2021

/csr needed

Loading

@openjdk openjdk bot added the csr label Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@dholmes-ora has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@navyxliu please create a CSR request and add link to it in JDK-8229517. This pull request cannot be integrated until the CSR request is approved.

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Mar 23, 2021

Hi, Reviewers,

I would like to restart the RFR process for the feature async logging. We (AWS) have deployed this feature over a year in a few critical services. It helps us to reduce long-tail GC pauses. On Linux, we used to experience intermittent second-level delays due to gclog writings. If those undesirable events happen to appear at safepoints, hotspot has to prolong the pause intervals, which then increase the response time of Java application/service.

Originally, we observed and solved this issue on a Linux system with software RAID. In absence of hardware assistance, multiple writes have to be synchronized and it is that operation that yields long pauses. This issue may become more prevalent if Linux servers adopt ZFS in the future. We don’t think redirecting log files to tmpfs is a final solution. Hotspot should provide a self-contained and cross-platform solution. Our solution is to provide a buffer and flush it in a standalone thread periodically.

Since then, we found more unexpected but interesting scenarios. e.g. some cloud-based applications run entirely on a AWS EBS partition. syscall write could be a blocking operation if the underlying infrastructure is experiencing an intermittent issue. Even stdout/stderr are not absolutely non-blocking. Someone may send XOFF of software flow control and pause the tty to read. As a result, the JVM process which is emitting logs to the tty is blocked then. Yes, that action may freeze the Java service accidentally!

Those pain points are not AWS-exclusive. We found relevant questions on stackoverflow[1] and it seems that J9 provides an option -Xgc:bufferedLogging to mitigate it[2]. We hope hotspot would consider our feature.

Back to implementation, this is the 2nd revision based on Unified logging. Previous RFR[3] was a top-down design. We provide a parallel header file asynclog.hpp and hope log-sites opt in. That design is very stiff because asynclog.hpp is full of template parameters and was turned down[4]. The new patch has deprecated the old design and achieved asynchronous logging in bottom-up way. We provide an output-option which conforms to JEP-158[5]. Developers can choose asynchronous mode for a file-based output by providing an extra option async=true. e.g. -Xlog:gc=debug:file=gc.log::async=true

May we know more about LogMessageBuffer.hpp/cpp? We haven’t found a real use of it. That’s why we are hesitating to support LogFileOutput::write(LogMessageBuffer::Iterator msg_iterator). Further, we haven’t supported async_mode for LogStdoutOutput and LogStderrOutput either. It’s not difficult but needs to big code change.

[1] https://stackoverflow.com/questions/27072340/is-gc-log-writing-asynchronous-safe-to-put-gc-log-on-nfs-mount
[2] https://stackoverflow.com/questions/54994943/is-openj9-gc-log-asynchronous
[3] https://cr.openjdk.java.net/~xliu/8229517/01/webrev/
[4] https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-March/041034.html
[5] https://openjdk.java.net/jeps/158

Loading

Copy link
Member

@YaSuenag YaSuenag left a comment

I think this PR is very useful for us!

May we know more about LogMessageBuffer.hpp/cpp? We haven’t found a real use of it. That’s why we are hesitating to support LogFileOutput::write(LogMessageBuffer::Iterator msg_iterator). Further, we haven’t supported async_mode for LogStdoutOutput and LogStderrOutput either. It’s not difficult but needs to big code change.

LogMessageBuffer is used in LogMessage. For example, we can see it as following. Frame # 1 is LogFileOutput::write(LogMessageBuffer::Iterator msg_iterator). IMHO we do not need to change LogStdout/errOutput, but it is better to change LogMessageBuffer.

#0  LogFileStreamOutput::write (this=this@entry=0x7ffff0002af0, msg_iterator=...)
    at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logDecorators.hpp:108
#1  0x00007ffff6e80e8e in LogFileOutput::write (this=this@entry=0x7ffff0002af0, msg_iterator=...)
    at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logFileOutput.cpp:314
#2  0x00007ffff6e876eb in LogTagSet::log (
    this=this@entry=0x7ffff7d4a640 <LogTagSetMapping<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::_tagset>, msg=...) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logTagSet.cpp:85
#3  0x00007ffff6a194df in LogImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::write (
    msg=...) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logTagSet.hpp:150
#4  LogMessageImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::flush (
    this=0x7ffff58675d0) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logMessage.hpp:79
#5  LogMessageImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::~LogMessageImpl (
    this=0x7ffff58675d0, __in_chrg=<optimized out>) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logMessage.hpp:74
#6  InstanceKlass::print_class_load_logging (this=this@entry=0x800007430, loader_data=loader_data@entry=0x7ffff00f5200,
    module_entry=module_entry@entry=0x0, cfs=cfs@entry=0x0) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.cpp:3647

Loading

src/hotspot/share/runtime/globals.hpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/runtime/globals.hpp Outdated Show resolved Hide resolved
Loading
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Mar 25, 2021

I think this PR is very useful for us!

May we know more about LogMessageBuffer.hpp/cpp? We haven’t found a real use of it. That’s why we are hesitating to support LogFileOutput::write(LogMessageBuffer::Iterator msg_iterator). Further, we haven’t supported async_mode for LogStdoutOutput and LogStderrOutput either. It’s not difficult but needs to big code change.

LogMessageBuffer is used in LogMessage. For example, we can see it as following. Frame # 1 is LogFileOutput::write(LogMessageBuffer::Iterator msg_iterator). IMHO we do not need to change LogStdout/errOutput, but it is better to change LogMessageBuffer.

#0  LogFileStreamOutput::write (this=this@entry=0x7ffff0002af0, msg_iterator=...)
    at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logDecorators.hpp:108
#1  0x00007ffff6e80e8e in LogFileOutput::write (this=this@entry=0x7ffff0002af0, msg_iterator=...)
    at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logFileOutput.cpp:314
#2  0x00007ffff6e876eb in LogTagSet::log (
    this=this@entry=0x7ffff7d4a640 <LogTagSetMapping<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::_tagset>, msg=...) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logTagSet.cpp:85
#3  0x00007ffff6a194df in LogImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::write (
    msg=...) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logTagSet.hpp:150
#4  LogMessageImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::flush (
    this=0x7ffff58675d0) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logMessage.hpp:79
#5  LogMessageImpl<(LogTag::type)16, (LogTag::type)68, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0, (LogTag::type)0>::~LogMessageImpl (
    this=0x7ffff58675d0, __in_chrg=<optimized out>) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/logging/logMessage.hpp:74
#6  InstanceKlass::print_class_load_logging (this=this@entry=0x800007430, loader_data=loader_data@entry=0x7ffff00f5200,
    module_entry=module_entry@entry=0x0, cfs=cfs@entry=0x0) at /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.cpp:3647

hi, @YaSuenag,

Thank you for providing the stacktrace! I didn't notice <logMessage.hpp> until you point out. Now I understand the rationale and usecases of logMessageBuffer. Let me figure out how to support it.

IIUC, the most important attribute of LogMessage is to guarantee messages are consecutive, or free from interleaving. I will focus on it.

Loading

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 25, 2021

Hi Xin,

I skimmed over the patch, but have a number of high level questions - things which have not been clear from your description.

  • Who does the writing, and who is affected when the writing stalls?
  • Do you then block or throw output away?
    • If the former, how do you mitigate the ripple effect?
    • If the latter, how does the reader of the log file know that something is missing?
  • How often do you flush? How do you prevent missing output in the log file in case of crashes?
  • Can this really the full brunt of logging (-Xlog:*=trace) over many threads?
  • Does this work with multiple target and multiple IO files?
  • Does it cost anything if logging is off or not async?

Update: Okay, I see you use PeriodicTask and the WatcherThread. Is this really enough? I would be concerned that it either runs too rarely to be able to swallow all output or that it runs that often that it monopolizes the WatcherThread.

I actually expected a separate Thread - or multiple, one per output - for this, waking up when there is something to write. That would also be more efficient than constant periodic polling.

  • How is the performance impact when we have lots of concurrent writes from many threads? I see that you use a Mutex to synchronize the logging threads with the flush service. Before, these threads would have done concurrent IO and that would be handled by the libc, potentially without locking.

I think this feature could be useful. I am a bit concerned with the increased complexity this brings. UL is already a very (I think unnecessarily) complex codebase. Maybe we should try to reduce its complexity first before adding new features to it. This is just my opinion, lets see what others think.

Cheers, Thomas

Loading

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 25, 2021

p.s. I like the integration into UL via a target modification btw. That feels very organic.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 26, 2021

Mailing list message from Thomas St��fe on hotspot-dev:

Can you please link the CSR to the issue?

On Fri, Mar 26, 2021 at 8:21 AM Yasumasa Suenaga <ysuenaga at openjdk.java.net>
wrote:

Loading

@robehn
Copy link
Contributor

@robehn robehn commented Mar 26, 2021

Hi,

This is flushed by the watcher thread (non-JavaThread).
Flushing can thus happen during a safepoint and one or more safepoints may have passed between the actual logging and the flushing.
If the VM thread logs it can be delayed while watcher thread does "pop_all()" it seems like.
I suppose pop_all can take a while if you have a couple of thousands of logs messages?

We can also change log-configuration during run-time, e.g. turn on/off logs via jcmd.
Wouldn't it be more natural to flush the async logs-lines before we update the log configuration? (e.g. if you turn off a log via jcmd, we flush the async buffer before)

Thanks

Loading

Copy link
Member

@simonis simonis left a comment

Hi Xin,

thanks for finally addressing this issue. In general your change looks good. Please find my detailed comments inline.

Best regards,
Volker

Loading

src/hotspot/share/runtime/globals.hpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logAsyncFlusher.hpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logAsyncFlusher.hpp Outdated Show resolved Hide resolved
Loading
return _tail == NULL ? NULL : _tail->peek();
}

void log_drop(E* e) {}
Copy link
Member

@simonis simonis Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on pop_front(). I'd remove this function entirely and handle it right in LogAsyncFlusher::enqueue().

Loading

src/hotspot/share/logging/logAsyncFlusher.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logAsyncFlusher.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logFileOutput.cpp Show resolved Hide resolved
Loading
src/hotspot/share/logging/logAsyncFlusher.hpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logFileOutput.cpp Outdated Show resolved Hide resolved
Loading
test/hotspot/gtest/logging/test_asynclog.cpp Outdated Show resolved Hide resolved
Loading
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Mar 26, 2021

hi, Thomas,

Thank you for reviewing this PR.

Hi Xin,

I skimmed over the patch, but have a number of high level questions - things which have not been clear from your description.

  • Who does the writing, and who is affected when the writing stalls?

The WatchThread eventually flushes those buffered messages. if the writing stalls, it blocks periodic tasks.
It blocks long enough, other periodic tasks are skipped.

  • Do you then block or throw output away?

    • If the former, how do you mitigate the ripple effect?
    • If the latter, how does the reader of the log file know that something is missing?

The capacity of buffer is limited, which is AsyncLogBufferSize (2k by default).
Actually, logTagSet.cpp limits the maximal length of a vwrite is 512. That means that maximal memory used by this buffer is 1M (=2k * 0.5k).

If the buffer overflows, it starts dropping the heads. this behavior simulates a ringbuffer.
If you enable -XX:+Verbose, the dropping message will be printed to the tty console.

I prefer to drop messages than keeping them growing because later may trigger out-of-memory error.

  • How often do you flush? How do you prevent missing output in the log file in case of crashes?

The interval is defined by LogAsyncInterval (300ms by default). I insert a statement async->flusher() in ostream_abort().

  • Can this really the full brunt of logging (-Xlog:*=trace) over many threads?
    to be honest, it can't. I see a lot of dropping messages on console with -XX:+Verbose.

I have tuned parameters that it won't drop messages easily for normal GC activity with info verbosity.
-Xlog:*=trace will drop messages indeed, but this is tunable. I have a stress test to tweak parameters.

  • Does this work with multiple target and multiple IO files?

Yes, it works if you have multiple outputs. LogAsyncFlusher is singleton. one single buffer and one thread serve them all.

  • Does it cost anything if logging is off or not async?

so far, LogAsyncFlusher as a periodic task remains active even no output is in async_mode.
it wakes up every LogAsyncInterval ms. it's a dummy task because the deque is always empty. the cost is almost nothing.

Update: Okay, I see you use PeriodicTask and the WatcherThread. Is this really enough? I would be concerned that it either runs too rarely to be able to swallow all output or that it runs that often that it monopolizes the WatcherThread.

I actually expected a separate Thread - or multiple, one per output - for this, waking up when there is something to write. That would also be more efficient than constant periodic polling.

You concern is reasonable. I don't understand why there is only one watchThread and up to 10 periodic tasks are crowded in it.
If it's a bottleneck, I plan to improve this infrastructure. I can make hotspot supports multiple watcher threads and spread periodic tasks among them. All watcher threads are connected using linked list to manage.

Can we treat it as a separated task? for normal usage, I think the delay is quite managed. Writing thousands of lines to a file usually can be done in sub-ms.

  • How is the performance impact when we have lots of concurrent writes from many threads? I see that you use a Mutex to synchronize the logging threads with the flush service. Before, these threads would have done concurrent IO and that would be handled by the libc, potentially without locking.

IMHO, logging shouldn't hurt performance a lot. At least, those do impact on performance are not supposed to enable by default. On the other side, I hope logging messages from other threads avoid from interweaving when I enable them to read.
That leads me to use mutex. That actually improves readability.

My design target is non-blocking. pop_all() is an ad-hoc operation which pop up all elements and release the mutex immediately. writeback() does IO without it.

In our real applications, we haven't seen this feature downgrade GC performance yet.

I think this feature could be useful. I am a bit concerned with the increased complexity this brings. UL is already a very (I think unnecessarily) complex codebase. Maybe we should try to reduce its complexity first before adding new features to it. This is just my opinion, lets see what others think.

Cheers, Thomas

I believe UL has its own reasons. In my defense, I don't make UL more complex. I only changed a couple of lines in one of its implementation file(logFileOutput.cpp) and didn't change its interfaces.

I try my best to reuse existing codebase. We can always refactor existing code(JDK-8239066, JDK-8263840), but it's not this PR's purpose.

thanks,
--lx

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Mar 27, 2021

Mailing list message from Thomas St��fe on hotspot-dev:

Can you please link the CSR to the issue?

On Fri, Mar 26, 2021 at 8:21 AM Yasumasa Suenaga
wrote:

https://bugs.openjdk.java.net/browse/JDK-8264323

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Hi Xin,

thank you for your detailed answers.

As I wrote, I think this is a useful change. A prior design discussion with a rough sketch would have made things easier. Also, it would have been good to have the CSR discussion beforehand, since it affects how complex the implementation needs to be. I don't know whether there had been design discussions beforehand; if I missed them, I apologize.

I am keenly aware that design discussions often lead nowhere because no-one answers. So I understand why you started with a patch.

About your proposal:

I do not think it can be made airtight, and I think that is okay - if we work with a limited flush buffer and we log too much, things will get dropped, that is unavoidable. But it has to be reliable and comprehensible after the fact.

As you write, the patch you propose works well with AWS, but I suspect that is an environment with limited variables, and outside use of the VM could be much more diverse. We must make sure to roll out only well designed solutions which work for us all.

E.g. a log system which randomly omits log entries because some internal buffer is full without giving any indication in the log itself is a terrible idea :). Since log files are a cornerstone for our support, I am interested in a good solution.

First off, the CSR:


  1. With what you propose, we could have a arbitrary combination of targets with different log files and different async options:
java -Xlog:os*:file=os.log::async=false -Xlog:os+thread:file=thread.log::async=true

Do we really need that much freedom? How probable is that someone wants different async options for different trace sinks? The more freedom we have here the more complex the implementation gets. All that stuff has to be tested. Why not just make "async" a global setting.

  1. AsyncLogBufferSize should be a user-definable memory size, not "number of slots". The fact that internally we keep a vector of disjunct memory snippets is an implementation detail; the user should just give a memory size and the VM should interprete this. This leaves us the freedom to later change the implementation as we see fit.

  2. LogAsyncInterval should not exist at all. If it has to exist, it should be a diagnostic switch, not a production one; but ideally, we would just log as soon as there is something to log, see below.


Implementation:

The use of the WatcherThread and PeriodicTask. Polling is plain inefficient, beside the concerns Robbin voiced about blocking things. This is a typical producer-consumer problem, and I would implement it using an own dedicated flusher thread and a monitor. The flusher thread should wake only if there is something to write. This is something I would not do in a separate RFE but now. It would also disarm any arguments against blocking the WatcherThread.


The fact that every log message gets strduped could be done better. This can be left for a future RFE - but it explains why I dislike "AsyncLogBufferSize" being "number of entries" instead of a memory size.

I think processing a memory-size AsyncLogBufferSize can be kept simple: it would be okay to just guess an average log line length and go with that. Lets say 256 chars. An AsyncLogBufferSize=1M could thus be translated to 4096 entries in your solution. If the sum of all 4096 allocated lines overshoots 1M from time to time, well so be it.

A future better solution could use a preallocated fixed sized buffer. There are two ways to do this, the naive but memory inefficient way - array of fixed sized text slots like the event system does. And a smart way: a ring buffer of variable sized strings, '\0' separated, laid out one after the other in memory. The latter is a bit more involved, but can be done, and it would be fast and very memory efficient. But as I wrote, this is an optimization which can be postponed.


I may misunderstand the patch, but do you resolve decorators when the flusher is printing? Would this not distort time-dependent decorators (timemillis, timenanos, uptime etc)? Since we record the time of printing, not the time of logging?.

If yes, it may be better to resolve the message early and just store the plain string and print that. Basically this would mean to move the whole buffering down a layer or two right at where the raw strings get printed. This would be vastly simplified if we abandon the "async for every trace sink" notion in favor of just a global flag.

This would also save a bit of space, since we would not have to carry all the meta information in AsyncLogMessage around. I count at least three 64bit slots, possibly 4-5, which alone makes for ~40 bytes per message. Resolved decorators are often smaller than that.

Please find further remarks inline.

hi, Thomas,

Thank you for reviewing this PR.

Hi Xin,
I skimmed over the patch, but have a number of high level questions - things which have not been clear from your description.

  • Who does the writing, and who is affected when the writing stalls?

The WatchThread eventually flushes those buffered messages. if the writing stalls, it blocks periodic tasks.
It blocks long enough, other periodic tasks are skipped.

  • Do you then block or throw output away?

    • If the former, how do you mitigate the ripple effect?
    • If the latter, how does the reader of the log file know that something is missing?

The capacity of buffer is limited, which is AsyncLogBufferSize (2k by default).
Actually, logTagSet.cpp limits the maximal length of a vwrite is 512. That means that maximal memory used by this buffer is 1M (=2k * 0.5k).

If the buffer overflows, it starts dropping the heads. this behavior simulates a ringbuffer.
If you enable -XX:+Verbose, the dropping message will be printed to the tty console.

I prefer to drop messages than keeping them growing because later may trigger out-of-memory error.

  • How often do you flush? How do you prevent missing output in the log file in case of crashes?

The interval is defined by LogAsyncInterval (300ms by default). I insert a statement async->flusher() in ostream_abort().

If the flusher blocks, this could block VM shutdown? Would this be different from what we do now, e.g. since all log output is serialized and done by one thread? Its probably fine, but we should think about this.

  • Can this really the full brunt of logging (-Xlog:*=trace) over many threads?
    to be honest, it can't. I see a lot of dropping messages on console with -XX:+Verbose.

I have tuned parameters that it won't drop messages easily for normal GC activity with info verbosity.
-Xlog:*=trace will drop messages indeed, but this is tunable. I have a stress test to tweak parameters.

  • Does this work with multiple target and multiple IO files?

Yes, it works if you have multiple outputs. LogAsyncFlusher is singleton. one single buffer and one thread serve them all.

The question was how we handle multiple trace sinks, see my "CSR" remarks.

  • Does it cost anything if logging is off or not async?

so far, LogAsyncFlusher as a periodic task remains active even no output is in async_mode.
it wakes up every LogAsyncInterval ms. it's a dummy task because the deque is always empty. the cost is almost nothing.

Update: Okay, I see you use PeriodicTask and the WatcherThread. Is this really enough? I would be concerned that it either runs too rarely to be able to swallow all output or that it runs that often that it monopolizes the WatcherThread.
I actually expected a separate Thread - or multiple, one per output - for this, waking up when there is something to write. That would also be more efficient than constant periodic polling.

You concern is reasonable. I don't understand why there is only one watchThread and up to 10 periodic tasks are crowded in it.
If it's a bottleneck, I plan to improve this infrastructure. I can make hotspot supports multiple watcher threads and spread periodic tasks among them. All watcher threads are connected using linked list to manage.

Can we treat it as a separated task? for normal usage, I think the delay is quite managed. Writing thousands of lines to a file usually can be done in sub-ms.

  • How is the performance impact when we have lots of concurrent writes from many threads? I see that you use a Mutex to synchronize the logging threads with the flush service. Before, these threads would have done concurrent IO and that would be handled by the libc, potentially without locking.

IMHO, logging shouldn't hurt performance a lot. At least, those do impact on performance are not supposed to enable by default. On the other side, I hope logging messages from other threads avoid from interweaving when I enable them to read.
That leads me to use mutex. That actually improves readability.

My design target is non-blocking. pop_all() is an ad-hoc operation which pop up all elements and release the mutex immediately. writeback() does IO without it.

Since you use a mutex it introduces synchronization, however short, across all logging threads. So it influences runtime behavior. For the record, I think this is okay; maybe a future RFE could improve this with a lockless algorithm. I just wanted to know if you measured anything, and I was curious whether there is a difference now between synchronous and asynchronous logging.

(Funnily, asynchronous logging is really more synchronous in a sense, since it synchronizes all logging threads across a common resource).

In our real applications, we haven't seen this feature downgrade GC performance yet.

I think this feature could be useful. I am a bit concerned with the increased complexity this brings. UL is already a very (I think unnecessarily) complex codebase. Maybe we should try to reduce its complexity first before adding new features to it. This is just my opinion, lets see what others think.
Cheers, Thomas

I believe UL has its own reasons. In my defense, I don't make UL more complex. I only changed a couple of lines in one of its implementation file(logFileOutput.cpp) and didn't change its interfaces.

I try my best to reuse existing codebase. We can always refactor existing code(JDK-8239066, JDK-8263840), but it's not this PR's purpose.

I understand. Its fine to do this in a later RFE.

thanks,
--lx

Cheers, Thomas

Loading

src/hotspot/share/logging/logAsyncFlusher.cpp Outdated Show resolved Hide resolved
Loading
Xin Liu added 2 commits Mar 28, 2021
LogMessage supports async_mode.
remove the option AsyncLogging
renanme  the option GCLogBufferSize to AsyncLogBufferSize
move drop_log() to LogAsyncFlusher.
add a constraint for the option LogAsyncInterval.
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Mailing list message from David Holmes on hotspot-dev:

Hi Xin,

On 27/03/2021 5:30 pm, Thomas Stuefe wrote:

On Mon, 22 Mar 2021 22:12:14 GMT, Xin Liu <xliu at openjdk.org> wrote:

This patch provides a buffer to store asynchrounous messages and flush them to
underlying files periodically.

IMO the discussions last year left it still an open question whether
this was a general purpose facility that we really needed/wanted in
hotspot. I think there is still a larger design discussion to be had for
a general purpose facility versus the in-house version you have been
using that suited your needs.

I'm piggy-backing on some of Thomas's comments below.

Hi Xin,

thank you for your detailed answers.

As I wrote, I think this is a useful change. A prior design discussion with a rough sketch would have made things easier. Also, it would have been good to have the CSR discussion beforehand, since it affects how complex the implementation needs to be. I don't know whether there had been design discussions beforehand; if I missed them, I apologize.

I am keenly aware that design discussions often lead nowhere because no-one answers. So I understand why you started with a patch.

About your proposal:

I do not think it can be made airtight, and I think that is okay - if we work with a limited flush buffer and we log too much, things will get dropped, that is unavoidable. But it has to be reliable and comprehensible after the fact.

As you write, the patch you propose works well with AWS, but I suspect that is an environment with limited variables, and outside use of the VM could be much more diverse. We must make sure to roll out only well designed solutions which work for us all.

E.g. a log system which randomly omits log entries because some internal buffer is full without giving any indication *in the log itself* is a terrible idea :). Since log files are a cornerstone for our support, I am interested in a good solution.

First off, the CSR:

---
1) With what you propose, we could have a arbitrary combination of targets with different log files and different async options:
java -Xlog:os*:file=os.log::async=false -Xlog:os+thread:file=thread.log::async=true

Do we really need that much freedom? How probable is that someone wants different async options for different trace sinks? The more freedom we have here the more complex the implementation gets. All that stuff has to be tested. Why not just make "async" a global setting.

Truly global or global for all actual file-based logging? I think
perhaps the latter. If we need per log file control that could be added
later if needed.

2) AsyncLogBufferSize should be a user-definable memory size, not "number of slots". The fact that internally we keep a vector of disjunct memory snippets is an implementation detail; the user should just give a memory size and the VM should interprete this. This leaves us the freedom to later change the implementation as we see fit.

I'm not sure it should be a bounded size at all. I don't like the idea
of needing yet another VM flag to control this. Why can't the design
accommodate unlimited buffer space as determined by available memory?

3) LogAsyncInterval should not exist at all. If it has to exist, it should be a diagnostic switch, not a production one; but ideally, we would just log as soon as there is something to log, see below.

The logging interval should be configurable IMO, so it either needs a
product switch, or preferably is set as a global logging option via the
Xlog command if that is possible.

---

Implementation:

The use of the WatcherThread and PeriodicTask. Polling is plain inefficient, beside the concerns Robbin voiced about blocking things. This is a typical producer-consumer problem, and I would implement it using an own dedicated flusher thread and a monitor. The flusher thread should wake only if there is something to write. This is something I would not do in a separate RFE but now. It would also disarm any arguments against blocking the WatcherThread.

I agree with Thomas here. Using the WatcherThread for this is not really
appropriate, though I understand it is convenient to use a thread that
is already present and, crucially, one that does not block for
safepoints. And we don't need multiple WatcherThreads or a way to
"spread the load" as the Watcher thread is very lightly loaded. So a new
kind of NonJavaThread should be introduced for this purpose - if it is
truly required - and directly synchronize with it rather than using
polling. Further, we must ensure that the flushing thread cannot block
the VMThread, if the VMThread is doing logging.

----

The fact that every log message gets strduped could be done better. This can be left for a future RFE - but it explains why I dislike "AsyncLogBufferSize" being "number of entries" instead of a memory size.

If we had had async logging from day one then the way we construct log
messages would have been done differently. With all the logging sites
assuming synchronous logging, with stack-local allocations/resources, we
have no choice but to copy messages to memory with a suitable lifetime.

As I started with, I think there needs to be a return to the high level
architectural design discussion for this feature. The PR serves as a
proof-of-concept, but should not IMO be presented as the right general
solution for all hotspot users.

Regards,
David
-----

Loading

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 29, 2021

Note that I am really in favor of bringing async logging to UL; this issue bopped up again and again, brought in various forms by various people. It will be good to finally tackle this.

But I agree that talking about the design first would be helpful. Maybe have a little mailing list thread to stop polluting this PR?

Loading

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 29, 2021

But I agree that talking about the design first would be helpful. Maybe have a little mailing list thread to stop polluting this PR?

I posted similar diacussion to hotspot-runtime-dev last November. It aims to implement to send UL via network socket. I believe this PR helps it.

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-November/043427.html

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Mar 30, 2021

But I agree that talking about the design first would be helpful. Maybe have a little mailing list thread to stop polluting this PR?

I posted similar diacussion to hotspot-runtime-dev last November. It aims to implement to send UL via network socket. I believe this PR helps it.

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-November/043427.html

Interesting. This design diagram is similar to this PR, but I don't think it is a good idea to have a blocking message buffer.
As mentioned in prior thread, it makes hotspot be more subject to external factors. TCP/UPD is an even more representative example of blocking IO than harddrive, isn't it?

Design and its Rationale

For async-logging feature, we proposed a lossy non-blocking design here. A bounded deque or ringbuffer gives a strong guarantee that log sites won't block java threads and the critical internal threads. This is the very problem we are meant to solve.

It can be proven that we cannot have all three guarantees at the same time: non-blocking, bounded memory and log fidelity. To overcome blocking I/O, which sometimes is not under our control, we think it's fair to trade log fidelity for non-blocking. If we kept fidelity and chose unbound buffer, we could end up with some spooky out-of-memory errors on some resource-constrained hardwares. We understand that the platforms hotspot running range from powerful servers to embedded devices. By leaving the buffer size adjustable, we can fit more scenarios. Nevertheless, with a bounded buffer, we believe developers can still capture important logging traits as long as the window is big enough and log messages are consecutive. The current implementation does provide those two merits.

A new proposal based on current implementation

I agree with reviewers' comments above. It's questionable to use the singleton WatcherThread to do IO-intensive job here. It may hinder other tasks. David's guess is right. I was not familiar with hotspot thread and quite frustrated to deal with a special-task thread's lifecycle. That why I used PeriodicTask. I feel more confident to take that challenge again.

Just like Yasumasa depicted, I can create a dedicated NonJavaThread to flush logs instead. Yesterday, I found WatcherThread::unpark() uses its monitor to wake up other pending tasks. I think we can implement in this way. Once log sites observe the buffer is half-full, it uses monitor::notify() to resume flusher thread. I think logging event is high-frequent but less contentious. Waking it up for each log message is not so economical. I have a lossy buffer anyway, so I suggest to have two checkpoints only: 1) half-full. 2) full.

Wrap it up

We would like to propose a lossy design of async-logging in this PR. It is a trade off, so I don't think it's a good idea to handle all logs in async mode. In practice, we hope people only choose async-logging for those logs which may happen at safepoints.

I understand Yasumasa's problem. If you would like to consider netcat or nfs/sshfs, I think your problem can still be solved in the existing file-based output. In this way, you can also utilize this feature by setting your "file" output async mode, then it makes your hotspot non-blocking over TCP as well.

Loading

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 30, 2021

This proposal mostly looks good to me, but it is better if async support is implement in higher level class.
LogFileOutput class inherited as following, and you modified LogFileOutput now (you might change LogFileStreamOutput later)

  • LogOutput
    • LogFileStreamOutput
      • LogFileOutput

I want to add async support to LogFileStreamOutput or LogFileStreamOutput because it helps us if we add other UL output (e.g. network socket) in future.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Mailing list message from David Holmes on hotspot-dev:

On 25/05/2021 3:51 pm, Xin Liu wrote:

On Tue, 25 May 2021 05:25:40 GMT, David Holmes <david.holmes at oracle.com> wrote:

I'm not sure what that means as I only expect there to be one timestamp.
I'm just not sure if it is recording the time at which the log call was
made, or the time (later) when that log record was actually written out?

Sorry. I updated the answer today. Let me elaborate it. Current implementation uses the original timestamps, which are accurate. When I create AsyncLogMessage instance, I copy _decorations.
@tstuefe recently compressed that class.

Using what applications and logging settings?

My observation is based on my integration test. Current semaphore-based synchronization can catch up the pace of gclog=trace.
https://github.com//pull/3135#issuecomment-827135465

If the buffer is nearly always empty and so it is very unlikely that the
final flush will have much, if anything to do, then arguably you don't
need the io_sem complexity.

_io_sem is introduced to pass unified logging test in async mode. I just enable them.
https://github.com//pull/3135/commits/2ffc33feaf5757bc5744440278e7750a9f2b81bf

It isn't at all clear why it was necessary to introduce the io_sem just
so some gtest will pass. Why did these tests fail?

Thanks,
David
-----

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Mailing list message from David Holmes on hotspot-dev:

On 25/05/2021 3:51 pm, Xin Liu wrote:

On Tue, 25 May 2021 05:25:40 GMT, David Holmes <david.holmes at oracle.com> wrote:

I'm not sure what that means as I only expect there to be one timestamp.
I'm just not sure if it is recording the time at which the log call was
made, or the time (later) when that log record was actually written out?

Sorry. I updated the answer today. Let me elaborate it. Current implementation uses the original timestamps, which are accurate. When I create AsyncLogMessage instance, I copy _decorations.
@tstuefe recently compressed that class.

Using what applications and logging settings?

My observation is based on my integration test. Current semaphore-based synchronization can catch up the pace of gclog=trace.
https://github.com//pull/3135#issuecomment-827135465

If the buffer is nearly always empty and so it is very unlikely that the
final flush will have much, if anything to do, then arguably you don't
need the io_sem complexity.

_io_sem is introduced to pass unified logging test in async mode. I just enable them.
https://github.com//pull/3135/commits/2ffc33feaf5757bc5744440278e7750a9f2b81bf

It isn't at all clear why it was necessary to introduce the io_sem just
so some gtest will pass. Why did these tests fail?

Thanks,
David
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Mailing list message from David Holmes on hotspot-dev:

On 25/05/2021 3:23 pm, Xin Liu 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().

You should have waited till there was further discussion on this as
there needs to be a consensus on what is needed here.

David
-----

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented May 25, 2021

It isn't at all clear why it was necessary to introduce the io_sem just
so some gtest will pass. Why did these tests fail?

You brought up this issue when you ran into crashes. #3135 (comment)
Here is my analysis. AsyncLogWriter::flush() was unsafe. Previously, flush() only flushed data out of the buffer. yes, it tries to write what it has dequeued, but there's a race condition I never think of!

Here is the case:
A thread invokes AsyncLogWriter::flush() and the buffer is empty at that time. Previously, write() returned because it had nothing to write. However, the following code in log writer thread may be executing in parallel!

  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);
    }
  }

If that race condition happens, it's unsafe to read log files after AsyncLogWriter::flush(). log writer thread is writing them!
It's even not safe to destroy LogOutput instances here. (That's the byproduct bug I discovered).
https://github.com/openjdk/jdk/pull/3135/files#diff-64ba5a15efefc31cf8bc2b1c51100933075913fc8ed6028ad348197e086c3c33R287

How to guarantee file writings have done? That's why I introduced _io_sem.
Now write() acquires and releases _io_sem no matter what. Even the buffer is empty.
This can guarantee that the buffer used to be empty and all dequeued messages are written by flush().

After I strengthen flush(), I found I could even enable all unified logging tests in async mode.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Mailing list message from Liu, Xin on hotspot-dev:

hi, David,

I thought of your concern. now I think your concern is valid.

If the underlying filesystem or socket is defunct, this line is may be
blocking. _io_sem is now zero. As a result, we can't finish aborting if
we do invoke AsyncLogWriter::flush() in aborting process.

e->output()->write_blocking(e->decorations(), msg);

Technically speaking, termination has the same issue.
We have to take the risk in termination, or the log files are not
integral. I think lost logs are acceptable in abortion scenario.

Thanks,
--lx

On 5/24/21 11:52 PM, David Holmes wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

On 25/05/2021 3:23 pm, Xin Liu 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().

You should have waited till there was further discussion on this as
there needs to be a consensus on what is needed here.

David
-----

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Mailing list message from Liu, Xin on hotspot-dev:

hi, David,

I thought of your concern. now I think your concern is valid.

If the underlying filesystem or socket is defunct, this line is may be
blocking. _io_sem is now zero. As a result, we can't finish aborting if
we do invoke AsyncLogWriter::flush() in aborting process.

e->output()->write_blocking(e->decorations(), msg);

Technically speaking, termination has the same issue.
We have to take the risk in termination, or the log files are not
integral. I think lost logs are acceptable in abortion scenario.

Thanks,
--lx

On 5/24/21 11:52 PM, David Holmes wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

On 25/05/2021 3:23 pm, Xin Liu 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().

You should have waited till there was further discussion on this as
there needs to be a consensus on what is needed here.

David
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 26, 2021

Mailing list message from David Holmes on hotspot-dev:

On 25/05/2021 5:30 pm, Xin Liu wrote:

On Tue, 25 May 2021 06:49:41 GMT, David Holmes <david.holmes at oracle.com> wrote:

It isn't at all clear why it was necessary to introduce the io_sem just
so some gtest will pass. Why did these tests fail?

You brought up this issue when you ran into crashes. https://github.com//pull/3135#issuecomment-845993038

Yes but I was unclear why these tests were failing the way they were.

Here is my analysis. AsyncLogWriter::flush() was unsafe. Previously, flush() only flushed data out of the buffer. yes, it try to write what it dequeued, but there's a race condition I didn't think of!

Here is the case:
A thread invokes AsyncLogWriter::flush() and the buffer is empty() at that time. Previously, write() returned because it had nothing to write. However, the following code in log writer thread may be executing in parallel!

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\)\;
 \}

}

If that race condition happens, it's unsafe to read log files after AsyncLogWriter::flush(). log writer thread is writing them!

If I understand what you are saying the issue was not the flush() itself
but the fact that we could continue past the flush() and call
LogConfiguration::finalize() while the writer thread was still writing
out messages.

So really what that was indicating was a failure to synchronize with the
writer thread during "shutdown". That could cause a crash or simply
result in missing log output.

It's even not safe to destroy LogOutput instances here. (That's the byproduct bug I discovered).
https://github.com//pull/3135/files#diff-64ba5a15efefc31cf8bc2b1c51100933075913fc8ed6028ad348197e086c3c33R287

How to guarantee file writings have done? That's why I introduced _io_sem.
Now write() acquires and releases _io_sem no matter what. Even the buffer is empty.
This can guarantee that the buffer used to be empty and all dequeued messages are written by flush().

Okay so let me walk through this:

- Some logging happens and writer thread is logically woken up but
doesn't yet execute.
- a shutdown occurs and we reach the point where we call
LogConfiguration::finalize() which calls disable_outputs()
- at this point no new logging is possible
- we then have a race between the shutdown thread calling flush() and
the async writer thread
- whomever gets the AsyncLogLock first claims all the buffered messages
and will write them out
- if the shutdown thread wins there is no problem, the async writer
thread will just redo _sem.wait()
- if the async writer thread wins we need the shutdown thread to wait
until the async writer has finished writing

So we could at this point do some direct kind of "handshake" between the
shutdown thread and the async writer thread to coordinate them. But
instead the _iosem is introduced. The async writer thread holds the
_iosem for the duration of the writing. The shutdown thread, after
finding no messages buffered, can't acquire the _iosem until the async
writer is done.

Okay I understand the protocol now.

After I strengthen `flush()`, I found I could even enable all unified logging tests in async mode.

So again prior to this the real problem was a race between final logging
output and VM termination, that could result in either crashes or lost
logs messages, which in turn could cause test failures.

At one stage I believe you had intended to terminate the async writer
thread, and that would have fixed the problem as well - assuming you
handled the termination properly.

Thanks,
David
-----

Loading

@simonis
Copy link
Member

@simonis simonis commented May 26, 2021

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?

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.

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 26, 2021

Mailing list message from David Holmes on hotspot-dev:

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.

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented May 26, 2021

So again prior to this the real problem was a race between final logging
output and VM termination, that could result in either crashes or lost
logs messages, which in turn could cause test failures.

For LogConfiguration::disable_outputs, it's not just for JVM termination.
When you send a dcmd VM.log disable to a running JVM, it also invokes it.

That bug caused JVM may crashes AND lost messages.
If flush() can't guarantee all dequeued messages have been written before returning, we may end up using broken pointers.

void LogConfiguration::disable_outputs() {
  size_t idx = _n_outputs;

  // Remove all outputs from all tagsets.
  for (LogTagSet* ts = LogTagSet::first(); ts != NULL; ts = ts->next()) {
    ts->disable_outputs();
  }

  // Handle jcmd VM.log disable
  // ts->disable_outputs() above has deleted output_list with RCU synchronization.
  // Therefore, no new logging entry can enter AsyncLog buffer for the time being.
  // flush pending entries before LogOutput instances die.
  AsyncLogWriter::flush();

  while (idx > 0) {
    LogOutput* out = _outputs[--idx];
    // Delete the output unless stdout or stderr (idx 0 or 1)
    if (idx > 1) {
      delete_output(idx);
    } else {
      out->set_config_string("all=off");
    }
  }
}

Those LogOutput instances are destroyed right after flush().
Each AsyncLogMessage instance contains a reference of LogFileOutput. LogWriter thread may use those deleted pointers!
that's the root cause you saw that intermittent crashes.

e->output()->write_blocking(e->decorations(), msg);

At one stage I believe you had intended to terminate the async writer
thread, and that would have fixed the problem as well - assuming you
handled the termination properly.

Yes, I used to have a termination protocol. It was also very complex because it must be MT-safe, no message lost and no concurrency with synchronous logging. Thomas disliked that and suggested me to remove the termination protocol(100+ line).

AsyncLogWriter::terminate() was placed before LogConfiguration::finalize(), therefore, it hid the problem in JVM termination.
It can't help for the dcmd situation, which we still need strong flush().The second use of strong flush() is for gtest. Without it, it's impossible to have a reliable way to check the results of async logging.

Loading

Copy link
Member

@simonis simonis left a comment

I'm fine with the latest version of this patch.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 27, 2021

Mailing list message from David Holmes on hotspot-dev:

Hi Xin,

On 27/05/2021 4:15 am, Xin Liu wrote:

On Wed, 26 May 2021 05:41:52 GMT, David Holmes <david.holmes at oracle.com> wrote:

So again prior to this the real problem was a race between final logging
output and VM termination, that could result in either crashes or lost
logs messages, which in turn could cause test failures.

For `LogConfiguration::disable_outputs`, it's not just for JVM termination.
When you send a dcmd `VM.log disable` to a running JVM, it also invokes it.

Thanks for highlighting that - yes that does make things additionally
complicated. So this one mechanism is addressing both cases and so
avoids the need for the additional termination protocol.

Thanks for bearing with me on this.

David
-----

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 27, 2021

Mailing list message from David Holmes on hotspot-dev:

Hi Xin,

On 27/05/2021 4:15 am, Xin Liu wrote:

On Wed, 26 May 2021 05:41:52 GMT, David Holmes <david.holmes at oracle.com> wrote:

So again prior to this the real problem was a race between final logging
output and VM termination, that could result in either crashes or lost
logs messages, which in turn could cause test failures.

For `LogConfiguration::disable_outputs`, it's not just for JVM termination.
When you send a dcmd `VM.log disable` to a running JVM, it also invokes it.

Thanks for highlighting that - yes that does make things additionally
complicated. So this one mechanism is addressing both cases and so
avoids the need for the additional termination protocol.

Thanks for bearing with me on this.

David
-----

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Nothing further from me. Other than two typos my comments hadn't yet been sent for.

Thanks,
David

Loading

src/hotspot/share/logging/logAsyncWriter.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/logging/logAsyncWriter.hpp Outdated Show resolved Hide resolved
Loading
Without that flush(), TEST='gtest:gtest:LogFileOutput*' can't pass
in async mode.
Loading
Copy link
Member

@simonis simonis left a comment

Still fine :)

Loading

Copy link
Member

@YaSuenag YaSuenag left a comment

Still good!

Loading

Copy link
Member

@phohensee phohensee left a comment

Still good.

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented May 27, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label May 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 27, 2021

@navyxliu
Your change (at version b3d36cf) is now ready to be sponsored by a Committer.

Loading

@phohensee
Copy link
Member

@phohensee phohensee commented May 27, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 27, 2021

@phohensee @navyxliu Since your change was applied there have been 270 commits pushed to the master branch:

  • 7c85f35: 8267123: Remove RMI Activation
  • 0754266: 8267709: Investigate differences between HtmlStyle and stylesheet.css
  • 23189a1: 8191786: Thread-SMR hash table size should be dynamic
  • ef368b3: 8265836: OperatingSystemImpl.getCpuLoad() returns incorrect CPU load inside a container
  • 10a6f5d: 8230623: Extract command-line help for -Xlint sub-options to new --help-lint
  • bea4109: 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar
  • ec65cf8: 8240347: remove undocumented options from jlink --help message
  • 3623abb: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
  • 85f6165: 8267817: [TEST] Remove unnecessary init in test/micro/org/openjdk/bench/javax/crypto/full/AESGCMBench:setup
  • 7278f56: 8267800: Remove the '_dirty' set in BCEscapeAnalyzer
  • ... and 260 more: https://git.openjdk.java.net/jdk/compare/04fad70437a43c0f38fd53414b8eace2eac93509...master

Your commit was automatically rebased without conflicts.

Pushed as commit 41185d3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment