This is an archive of the discontinued LLVM Phabricator instance.

[Support] Make raw_string_ostream unbuffered
AbandonedPublic

Authored by lebedev.ri on Feb 25 2019, 12:20 PM.

Details

Summary

In D58580 i have noted that llvm::to_string() is a memory hog.
It uses raw_string_ostream, and since it was buffered,
every raw_string_ostream had a cost of BUFSIZ bytes
(which is 8192 at least here). So every llvm::to_string()
call, even to just print an int, costed 8192 bytes.

In D58580, getting rid of that buffering had significant
performance and memory consumption improvements for llvm-xray convert.

Similarly, in D58580 @rnk pointed out that the raw_svector_ostream
is already unbuffered, and write_unsigned_impl and friends
do internal buffering. So it should be ok to also make
raw_string_ostream unbuffered.

Here, i don't have any perf measurements.
Another letdown is that i'm leaving a loose-end - not deleting the flush() method.
I don't expect that cleanup to be anything more than just fixing every
new compiler error, but i'm presently unable to do that.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Feb 25 2019, 12:20 PM
rnk accepted this revision.Feb 25 2019, 12:43 PM

lgtm

Leaving flush alone is fine.

include/llvm/Support/raw_ostream.h
497

Huh, I suppose that is nicer than calling SetUnbuffered.

This revision is now accepted and ready to land.Feb 25 2019, 12:43 PM
lebedev.ri marked an inline comment as done.Feb 25 2019, 12:48 PM
In D58643#1409649, @rnk wrote:

lgtm

Thank you for the review.

Leaving flush alone is fine.

The problem being that i still haven't migrated to the monorepo.
Hm, actually, i think i can do it (well, except lldb), simply by doing all the changes,
committing them, and then committing the removal of the method itself last.
I'll look into that later.

include/llvm/Support/raw_ostream.h
497

Thanks

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Feb 25 2019, 1:21 PM

Uhm, but if we disable buffering, and cripple flush(), shouldn't we also make it impossible to turn the buffering back on?
Existing test fails with this change, and if i add similar test with SmallString/raw_svector_ostream it also fails in trunk.

TEST(raw_ostreamTest, TinyBuffer) {
  std::string Str;
  raw_string_ostream OS(Str);
  OS.SetBufferSize(1);
  OS << "hello";
  OS << 1;
  OS << 'w' << 'o' << 'r' << 'l' << 'd';
  EXPECT_EQ("hello1world", OS.str());
}

TEST(raw_svector_ostreamTest, TinyBuffer) {
  llvm::SmallString<0> Str;
  raw_svector_ostream OS(Str);
  OS.SetBufferSize(1);
  OS << "hello";
  OS << 1;
  OS << 'w' << 'o' << 'r' << 'l' << 'd';
  EXPECT_EQ("hello1world", OS.str());
}
$ unittests/Support/SupportTests --gtest_filter=*TinyBuffer
Note: Google Test filter = *TinyBuffer
[==========] Running 2 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from raw_ostreamTest
[ RUN      ] raw_ostreamTest.TinyBuffer
/build/llvm/unittests/Support/raw_ostream_test.cpp:127: Failure
      Expected: "hello1world"
To be equal to: OS.str()
      Which is: "hello1worl"
[  FAILED  ] raw_ostreamTest.TinyBuffer (0 ms)
[----------] 1 test from raw_ostreamTest (0 ms total)

[----------] 1 test from raw_svector_ostreamTest
[ RUN      ] raw_svector_ostreamTest.TinyBuffer
/build/llvm/unittests/Support/raw_ostream_test.cpp:137: Failure
      Expected: "hello1world"
      Which is: 0x55cc2aeca73f
To be equal to: OS.str()
      Which is: { 'h' (104, 0x68), 'e' (101, 0x65), 'l' (108, 0x6C), 'l' (108, 0x6C), 'o' (111, 0x6F), '1' (49, 0x31), 'w' (119, 0x77), 'o' (111, 0x6F), 'r' (114, 0x72), 'l' (108, 0x6C) }
SupportTests: /build/llvm/lib/Support/raw_ostream.cpp:72: virtual llvm::raw_ostream::~raw_ostream(): Assertion `OutBufCur == OutBufStart && "raw_ostream destructor called with non-empty buffer!"' failed.
 #0 0x00007fb00164150a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/build/llvm-build-GCC-release/lib/libLLVMSupport.so.9svn+0x1b150a)
 #1 0x00007fb00163f2f4 llvm::sys::RunSignalHandlers() (/build/llvm-build-GCC-release/lib/libLLVMSupport.so.9svn+0x1af2f4)
 #2 0x00007fb00163f455 SignalHandler(int) (/build/llvm-build-GCC-release/lib/libLLVMSupport.so.9svn+0x1af455)
 #3 0x00007fb0014426e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x126e0)
 #4 0x00007fb000eff8bb gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x378bb)
 #5 0x00007fb000eea535 abort (/lib/x86_64-linux-gnu/libc.so.6+0x22535)
 #6 0x00007fb000eea40f (/lib/x86_64-linux-gnu/libc.so.6+0x2240f)
 #7 0x00007fb000ef80f2 (/lib/x86_64-linux-gnu/libc.so.6+0x300f2)
 #8 0x00007fb00161fd40 llvm::raw_svector_ostream::~raw_svector_ostream() (/build/llvm-build-GCC-release/lib/libLLVMSupport.so.9svn+0x18fd40)
 #9 0x000055cc2b1fc226 (anonymous namespace)::raw_svector_ostreamTest_TinyBuffer_Test::TestBody() (unittests/Support/SupportTests+0x37c226)
#10 0x00007fb00140a98a testing::Test::Run() (/build/llvm-build-GCC-release/lib/libgtest.so.9svn+0x4298a)
#11 0x00007fb00140aad0 testing::TestInfo::Run() (/build/llvm-build-GCC-release/lib/libgtest.so.9svn+0x42ad0)
#12 0x00007fb00140ab95 testing::TestCase::Run() (/build/llvm-build-GCC-release/lib/libgtest.so.9svn+0x42b95)
#13 0x00007fb00140b05c testing::internal::UnitTestImpl::RunAllTests() (/build/llvm-build-GCC-release/lib/libgtest.so.9svn+0x4305c)
#14 0x00007fb00140b147 testing::UnitTest::Run() (/build/llvm-build-GCC-release/lib/libgtest.so.9svn+0x43147)
#15 0x00007fb001429161 main (/build/llvm-build-GCC-release/lib/libgtest_main.so.9svn+0x1161)
#16 0x00007fb000eec09b __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409b)
#17 0x000055cc2af6c02a _start (unittests/Support/SupportTests+0xec02a)
Aborted
This revision is now accepted and ready to land.Feb 25 2019, 1:21 PM
lebedev.ri planned changes to this revision.Feb 25 2019, 1:29 PM
rnk added a comment.Feb 25 2019, 1:37 PM

Uhm, but if we disable buffering, and cripple flush(), shouldn't we also make it impossible to turn the buffering back on?
Existing test fails with this change, and if i add similar test with SmallString/raw_svector_ostream it also fails in trunk.

Can work around this by calling this->raw_ostream::flush() from str() and ~raw_string_ostream?

lebedev.ri abandoned this revision.Jan 25 2020, 3:09 AM