[Support] Move raw_ostream::tie to raw_fd_ostream #97396
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Originally, tie was introduced by D81156 to flush stdout before writing to stderr. 0308975 reverted this due to race conditions. Nonetheless, it does cost performance, causing an extra check in the "cold" path, which is actually the hot path for raw_svector_ostream. Given that this feature sees almost no use, remove it. Reverts commit 1ce8319.
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-clang-tools-extra Author: Alexis Engelke (aengelke) ChangesOriginally, tie was introduced by D81156 to flush stdout before writing to stderr. 0308975 reverted this due to race conditions. Nonetheless, it does cost performance, causing an extra check in the "cold" path, which is actually the hot path for raw_svector_ostream. Given that this feature sees almost no use, remove it. Reverts commit 1ce8319. What was the motivation to keep this in llvm-dwarfdump? Is there any other way to achieve the same goal? Full diff: https://github.com/llvm/llvm-project/pull/97396.diff 6 Files Affected:
diff --git a/clang-tools-extra/clangd/index/remote/server/Server.cpp b/clang-tools-extra/clangd/index/remote/server/Server.cpp
index 4ef3ab6f9af9c..52fca53260a16 100644
--- a/clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ b/clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -499,8 +499,6 @@ int main(int argc, char *argv[]) {
}
llvm::errs().SetBuffered();
- // Don't flush stdout when logging for thread safety.
- llvm::errs().tie(nullptr);
auto Logger = makeLogger(LogPrefix.getValue(), llvm::errs());
clang::clangd::LoggingSession LoggingSession(*Logger);
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index c3ba655ee2dc6..73000d96c6ca8 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -840,8 +840,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
// Use buffered stream to stderr (we still flush each log message). Unbuffered
// stream can cause significant (non-deterministic) latency for the logger.
llvm::errs().SetBuffered();
- // Don't flush stdout when logging, this would be both slow and racy!
- llvm::errs().tie(nullptr);
StreamLogger Logger(llvm::errs(), LogLevel);
LoggingSession LoggingSession(Logger);
// Write some initial logs before we start doing any real work.
diff --git a/llvm/include/llvm/Support/raw_ostream.h b/llvm/include/llvm/Support/raw_ostream.h
index 0951ffb19ffa1..012915d341634 100644
--- a/llvm/include/llvm/Support/raw_ostream.h
+++ b/llvm/include/llvm/Support/raw_ostream.h
@@ -82,10 +82,6 @@ class raw_ostream {
char *OutBufStart, *OutBufEnd, *OutBufCur;
bool ColorEnabled = false;
- /// Optional stream this stream is tied to. If this stream is written to, the
- /// tied-to stream will be flushed first.
- raw_ostream *TiedStream = nullptr;
-
enum class BufferKind {
Unbuffered = 0,
InternalBuffer,
@@ -360,10 +356,6 @@ class raw_ostream {
bool colors_enabled() const { return ColorEnabled; }
- /// Tie this stream to the specified stream. Replaces any existing tied-to
- /// stream. Specifying a nullptr unties the stream.
- void tie(raw_ostream *TieTo) { TiedStream = TieTo; }
-
//===--------------------------------------------------------------------===//
// Subclass Interface
//===--------------------------------------------------------------------===//
@@ -422,9 +414,6 @@ class raw_ostream {
/// flushing. The result is affected by calls to enable_color().
bool prepare_colors();
- /// Flush the tied-to stream (if present) and then write the required data.
- void flush_tied_then_write(const char *Ptr, size_t Size);
-
virtual void anchor();
};
diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp
index 0acb54f76c0bf..b202d0fe5a687 100644
--- a/llvm/lib/Support/raw_ostream.cpp
+++ b/llvm/lib/Support/raw_ostream.cpp
@@ -221,7 +221,7 @@ void raw_ostream::flush_nonempty() {
assert(OutBufCur > OutBufStart && "Invalid call to flush_nonempty.");
size_t Length = OutBufCur - OutBufStart;
OutBufCur = OutBufStart;
- flush_tied_then_write(OutBufStart, Length);
+ write_impl(OutBufStart, Length);
}
raw_ostream &raw_ostream::write(unsigned char C) {
@@ -229,7 +229,7 @@ raw_ostream &raw_ostream::write(unsigned char C) {
if (LLVM_UNLIKELY(OutBufCur >= OutBufEnd)) {
if (LLVM_UNLIKELY(!OutBufStart)) {
if (BufferMode == BufferKind::Unbuffered) {
- flush_tied_then_write(reinterpret_cast<char *>(&C), 1);
+ write_impl(reinterpret_cast<char *>(&C), 1);
return *this;
}
// Set up a buffer and start over.
@@ -249,7 +249,7 @@ raw_ostream &raw_ostream::write(const char *Ptr, size_t Size) {
if (LLVM_UNLIKELY(size_t(OutBufEnd - OutBufCur) < Size)) {
if (LLVM_UNLIKELY(!OutBufStart)) {
if (BufferMode == BufferKind::Unbuffered) {
- flush_tied_then_write(Ptr, Size);
+ write_impl(Ptr, Size);
return *this;
}
// Set up a buffer and start over.
@@ -265,7 +265,7 @@ raw_ostream &raw_ostream::write(const char *Ptr, size_t Size) {
if (LLVM_UNLIKELY(OutBufCur == OutBufStart)) {
assert(NumBytes != 0 && "undefined behavior");
size_t BytesToWrite = Size - (Size % NumBytes);
- flush_tied_then_write(Ptr, BytesToWrite);
+ write_impl(Ptr, BytesToWrite);
size_t BytesRemaining = Size - BytesToWrite;
if (BytesRemaining > size_t(OutBufEnd - OutBufCur)) {
// Too much left over to copy into our buffer.
@@ -306,12 +306,6 @@ void raw_ostream::copy_to_buffer(const char *Ptr, size_t Size) {
OutBufCur += Size;
}
-void raw_ostream::flush_tied_then_write(const char *Ptr, size_t Size) {
- if (TiedStream)
- TiedStream->flush();
- write_impl(Ptr, Size);
-}
-
// Formatted output.
raw_ostream &raw_ostream::operator<<(const format_object_base &Fmt) {
// If we have more than a few bytes left in our output buffer, try
diff --git a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
index d00cf52075712..d5f5e6fe37b78 100644
--- a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
+++ b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
@@ -821,10 +821,6 @@ static bool handleFile(StringRef Filename, HandlerFn HandleObj,
int main(int argc, char **argv) {
InitLLVM X(argc, argv);
- // Flush outs() when printing to errs(). This avoids interleaving output
- // between the two.
- errs().tie(&outs());
-
llvm::InitializeAllTargetInfos();
llvm::InitializeAllTargetMCs();
diff --git a/llvm/unittests/Support/raw_ostream_test.cpp b/llvm/unittests/Support/raw_ostream_test.cpp
index 451eda8af51b6..cfd00f212ef11 100644
--- a/llvm/unittests/Support/raw_ostream_test.cpp
+++ b/llvm/unittests/Support/raw_ostream_test.cpp
@@ -382,88 +382,6 @@ TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
{ raw_fd_ostream("-", EC, sys::fs::OpenFlags::OF_None); }
}
-TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
- std::string TiedToBuffer;
- raw_string_ostream TiedTo(TiedToBuffer);
- TiedTo.SetBuffered();
- TiedTo << "a";
-
- std::string Buffer;
- raw_string_ostream TiedStream(Buffer);
- TiedStream.tie(&TiedTo);
- // Sanity check that the stream hasn't already been flushed.
- EXPECT_EQ("", TiedToBuffer);
-
- // Empty string doesn't cause a flush of TiedTo.
- TiedStream << "";
- EXPECT_EQ("", TiedToBuffer);
-
- // Non-empty strings trigger flush of TiedTo.
- TiedStream << "abc";
- EXPECT_EQ("a", TiedToBuffer);
-
- // Single char write flushes TiedTo.
- TiedTo << "c";
- TiedStream << 'd';
- EXPECT_EQ("ac", TiedToBuffer);
-
- // Write to buffered stream without flush does not flush TiedTo.
- TiedStream.SetBuffered();
- TiedStream.SetBufferSize(2);
- TiedTo << "e";
- TiedStream << "f";
- EXPECT_EQ("ac", TiedToBuffer);
-
- // Explicit flush of buffered stream flushes TiedTo.
- TiedStream.flush();
- EXPECT_EQ("ace", TiedToBuffer);
-
- // Explicit flush of buffered stream with empty buffer does not flush TiedTo.
- TiedTo << "g";
- TiedStream.flush();
- EXPECT_EQ("ace", TiedToBuffer);
-
- // Write of data to empty buffer that is greater than buffer size flushes
- // TiedTo.
- TiedStream << "hijklm";
- EXPECT_EQ("aceg", TiedToBuffer);
-
- // Write of data that overflows buffer size also flushes TiedTo.
- TiedStream.flush();
- TiedStream << "n";
- TiedTo << "o";
- TiedStream << "pq";
- EXPECT_EQ("acego", TiedToBuffer);
-
- // Streams can be tied to each other safely.
- TiedStream.flush();
- Buffer = "";
- TiedTo.tie(&TiedStream);
- TiedTo.SetBufferSize(2);
- TiedStream << "r";
- TiedTo << "s";
- EXPECT_EQ("", Buffer);
- EXPECT_EQ("acego", TiedToBuffer);
- TiedTo << "tuv";
- EXPECT_EQ("r", Buffer);
- TiedStream << "wxy";
- EXPECT_EQ("acegostuv", TiedToBuffer);
- // The x remains in the buffer, since it was written after the flush of
- // TiedTo.
- EXPECT_EQ("rwx", Buffer);
- TiedTo.tie(nullptr);
-
- // Calling tie with nullptr unties stream.
- TiedStream.SetUnbuffered();
- TiedStream.tie(nullptr);
- TiedTo << "y";
- TiedStream << "0";
- EXPECT_EQ("acegostuv", TiedToBuffer);
-
- TiedTo.flush();
- TiedStream.flush();
-}
-
TEST(raw_ostreamTest, reserve_stream) {
std::string Str;
raw_string_ostream OS(Str);
|
If warnings or errors are printed by llvm-dwarfdump without this, we end up with corrupted output, where stderr and stdout are intermixed in a semi-arbitrary way. It was a while back that I made the original change mind you, so I don't have the concrete example in front of me any more. Looking at discussions in the original reviews on related commits make me think llvm-dwarfdump's --debug-line option was particularly badly affected: I'd be surprised if there aren't tests that start failing (potentially flakily, given that it'll depend on buffer sizes and therefore the amount of data written, which may be platform-specific etc) because of this change. In practice, I wanted this enabled for all tools however, it just never happened.
Unless the additional check is causing serious issues, I'm not sure this is the right question (and even if it is the right question, I think it's on you as the person proposing to remove the change to provide an alternative - I don't really work on LLVM beyond reviewing these days, so I don't have the time to consider alternatives to work that is currently doing the job well for at least one initial use-case I care about). What I feel the right initial question is, is this check actually making a significant difference to performance in real-world use cases? |
raw_ostream::write shows up in my (downstream application) profiles at ~0.6%, mostly through raw_svector_ostream, which is always unbuffered. The performance improvement of this one-condition-removal is measureable (c-t-t). But I see your point. I reworked this so that tie() still works, but only on raw_fd_ostream (errs() is an raw_fd_ostream), so that other streams (svector, string) get performance gains. I also updated the comment to state the motivation/use case for this feature. |
aengelke
changed the title
|
Unneeded |
MaskRay
approved these changes
Jul 2, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/812 Here is the relevant piece of the build log for the reference: |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
Originally, tie was introduced by D81156 to flush stdout before writing to stderr. 0308975 reverted this due to race conditions. Nonetheless, it does cost performance, causing an extra check in the "cold" path, which is actually the hot path for raw_svector_ostream. Given that this feature is only used for errs(), move it to raw_fd_ostream so that it no longer affects performance of other stream classes.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
Originally, tie was introduced by D81156 to flush stdout before writing to stderr. 0308975 reverted this due to race conditions. Nonetheless, it does cost performance, causing an extra check in the "cold" path, which is actually the hot path for raw_svector_ostream. Given that this feature is only used for errs(), move it to raw_fd_ostream so that it no longer affects performance of other stream classes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally, tie was introduced by D81156 to flush stdout before writing to stderr. 0308975 reverted this due to race conditions. Nonetheless, it does cost performance, causing an extra check in the "cold" path, which is actually the hot path for raw_svector_ostream. Given that this feature is only used for errs(), move it to raw_fd_ostream so that it no longer affects performance of other stream classes.