[llvm/Support/VirtualOutputBackends] Don't create unbuffered streams when the MirroringOutputBackend is used

[akyrtzi]

MirroringOutputBackend was forcing the raw_ostreams to be unbuffered, causing significant slowdown due to I/O. When enabling Clang caching for building Clang or WebKit, the "from scratch" (all cache misses) build was slower than the regular non-caching build. For building debug Clang the overhead was ~22%, and for debug build of WebKit, it was ~15%.

The overhead went away after removing the SetUnbuffered() calls.

rdar://130514092

[akyrtzi]

@swift-ci Please test LLVM

[akyrtzi]

I put back the flush(); call in pwrite_impl(), otherwise SupportTests/VirtualOutputBackendAdaptors/makeMirroringOutputBackend unit test is failing.

[akyrtzi]

@swift-ci Please test LLVM

[cachemeifyoucan]

I put back the flush(); call in pwrite_impl(), otherwise SupportTests/VirtualOutputBackendAdaptors/makeMirroringOutputBackend unit test is failing.

If it is only that test, it is fine to flush in the unit test before checking content.

[akyrtzi]

If it is only that test, it is fine to flush in the unit test before checking content.

My worry is that the test failure is indication that the flush call is necessary to support the behavioral expectations of pwrite callers.

MirroringOutput::pwrite_impl() is not called during compilation so I'm not so worried about it, we can revisit after input from @dexonsmith

[dexonsmith]

The unbuffered behaviour was intentional, and it seemed important, but I can't remember why... it's possible it only mattered for some clients (and it's possible those clients were hypothetical).

Maybe this was for live display of command-line output? (Or stderr?)

Or maybe it was something more subtle, like ensuring there's only one buffer, rather than a chain of buffers... at some point I had to do funny things with buffers to fix a weird crash after compilation failed.

[akyrtzi]

@dexonsmith could you comment on this override of MirroringOutput:

    void pwrite_impl(const char *Ptr, size_t Size, uint64_t Offset) override {
      this->flush();
      F1->getOS().pwrite(Ptr, Size, Offset);
      F2->getOS().pwrite(Ptr, Size, Offset);
    }

pwrite_impl() is not called during compilation but I was worried the flush() call may cause some performance issue if it does get used. I did notice that the SupportTests/VirtualOutputBackendAdaptors/makeMirroringOutputBackend unit test depends on the presence of the flush() call.

Should we not be worried about performance issues related to calling flush() for each pwrite call?