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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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

> In D90010#2355460 <https://reviews.llvm.org/D90010#2355460>, @dblaikie wrote:
>
>> 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)
>
> Sure
>
>> 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.
>
> I tried setting...
> static void printStats(const ClangTidyStats &Stats) {
> +  llvm::errs().SetBufferSize(4096);
> +  llvm::errs().SetBuffered();
>
> but still didn't work!

By the looks of the code, you may want to call SetBufferSize only (do not call SetBuffered after that - or it'll go back to the default buffer size of 0.

>>> 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?)
>
> It effectively slows down the processing of files.

Do you have any measurements of how much it slows things down/how much the patch you're proposing speeds things up?

> Same is there for stdout too.
>
> For stderr messages, as you suggested, we may need option to switch off certain (stderr) messages.
> Can you please suggest based on which condition we can switch it off?

Looks like, judging by the code above on line 481/482, that --quiet would turn off this stat printing stuff - but also, these stats are only printed if there were ignored errors, by the looks of it? Are you trying to run clang-tidy over a codebase without the correct flags for your codebase, or is there some other reason clang-tidy would be printing/ignoring a lot of errors?

In any case, I'd suggest either fixing the errors or, if you've decided the errors are acceptable for some reason, using --quiet to silence the errors you aren't interested in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010