[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D90010#2355443 <https://reviews.llvm.org/D90010#2355443>, @Hiralo wrote:

> In D90010#2355432 <https://reviews.llvm.org/D90010#2355432>, @dblaikie wrote:
>
>> Looks like you might be able to do something like "llvm::errs().setBuffered()" ?
>
> Do we need to set it for each function using llvm:errs() (e.g. here printStats() )
> OR can it be set once for entire clang-tidy ?

I think it could be set for the whole process. (though there's a reason stderr isn't usually buffered - because if it's buffered and the process crashes, then you don't get all the output/maybe something important will be lost)

>>> In D90010#2352627 <https://reviews.llvm.org/D90010#2352627>, @dblaikie wrote:
>>>
>>>> (the patch description doesn't explain any specific motivation either - whether it's performance (runtime? memory usage? etc?) or something else, and how the performance aspect has been quantified)
>>>
>>> The motivation is to avoid 7 write calls which helps in large build system and easy on NFS!
>>
>> Sorry, I meant "why does any of this matter" I take it you mean "because 7 write calls are slower than 1 in <this situation> by <this much time/percentage>" - do you have rough data/description of the situation where the speed of printing error messages matters and by how much does it matter? (I think it would be good to have this data no matter the solution - be it explicit or built-in buffering)
>
> No worries. Thanks for your valuable reviews.
>
> For example, consider clang-tidy running on 10,000 files... we expect to have minimal number of write calls. With the code-as-is makes 10,000 * 7 = 70,000 stderr write calls with small small chunk of strings!!!
> With proposed changes OR with llvm::errs().setBuffered() set we will see only 10,000 legitimate stderr write calls :)

Right, but what I mean is "what observable difference do you see" - as a user you aren't counting write calls, you're looking at the wall time, for instance. What difference in user-observable behavior are you seeing/are you trying to address, and how much does this change help that user-observable behavior?

I don't know too much about clang-tidy, but the statistics output for 10,000 files doesn't seem like it'd be that useful, would it? So maybe the solution is to turn it off, rather than to make it faster? (generally error paths are thought of as not needing to be efficient - because we shouldn't be producing thousands of errors to be consumed, because readers aren't going to read thousands of errors, usually? Are your users/you reading all the output that clang-tidy is generating?)

In D90010#2355459 <https://reviews.llvm.org/D90010#2355459>, @Hiralo wrote:

> Tried using llvm::errs().SetBuffered() within printStats()...
>
> <diff>
> static void printStats(const ClangTidyStats &Stats) {
> +  llvm::errs().SetBuffered()
> </diff>
>
> but still I see below stderr write calls...
> ...
> write(2, "10712", 5)                    = 5
> write(2, " warning", 8)                 = 8
> write(2, "s", 1)                        = 1
> write(2, " generated", 10)              = 10
> write(2, ".\n", 2)                      = 2
> ...
> write(2, "Suppressed ", 11)             = 11
> write(2, "10703", 5)                    = 5
> write(2, " warnings (", 11)             = 11
> write(2, "10703", 5)                    = 5
> write(2, " in non-user code", 17)       = 17
> write(2, ").\n", 3)                     = 3
> write(2, "Use -header-filter=.* to display"..., 136) = 136
> ...
>
> We expect it to emit one write call only!

Might be worth debugging through it and seeing what's happening, perhaps? I'm not sure exactly why that might not be buffering - perhaps it uses  raw_fd_ostream::preferred_buffer_size which might be returning 0 if it's a terminal that it's outputting to? In that case you could try "SetBufferSize" that'll set a customized buffer size and, I think, enable buffering.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90010/new/

https://reviews.llvm.org/D90010