[PATCH] D87272: [lld] Buffer writes when composing a single diagnostic

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D87272#2264430 <https://reviews.llvm.org/D87272#2264430>, @dblaikie wrote:

> In D87272#2264421 <https://reviews.llvm.org/D87272#2264421>, @MaskRay wrote:
>
>> In D87272#2264383 <https://reviews.llvm.org/D87272#2264383>, @dblaikie wrote:
>>
>>> In D87272#2263758 <https://reviews.llvm.org/D87272#2263758>, @MaskRay wrote:
>>>
>>>> In D87272#2261876 <https://reviews.llvm.org/D87272#2261876>, @dblaikie wrote:
>>>>
>>>>> In D87272#2261812 <https://reviews.llvm.org/D87272#2261812>, @MaskRay wrote:
>>>>>
>>>>>> In D87272#2261768 <https://reviews.llvm.org/D87272#2261768>, @dblaikie wrote:
>>>>>>
>>>>>>> Is performance important in this case? (admittedly, some folks might have builds full of warnings they aren't paying attention to & so they still care about performance then?) Generally the idea with clang/etc is that anything that produces user output (other than the successfully compiled/linked file) doesn't need to be fast (in compile/link-time terms) because it'll take /way/ longer for a user to read and think about the error message than the slight increase in execution time.
>>>>>>
>>>>>> The nicer thing is that when running under strace/truss, there will be just one write for one diagnostic.
>>>>>
>>>>> Is that true of many other tools? (I wouldn't've expected it'd be true of clang, for instance - or probably other LLVM utilities) - is it especially helpful?
>>>>
>>>> The motivation was to reduce unneeded ::write calls.
>>>
>>> Sounds like unnecessary/premature optimization, though?
>>
>> I don't think it is premature optimization. When a --symbol-ordering-file has absent entries or when --call-graph-ordering-file has incorrect entries, the terminal output can be quite slow. This change can make it much faster by also making things just better (one write syscall for one line of diagnostic).
>
> How much faster is the performance if you user disables the warning they have chosen not to fix? (if they're fixing it, the time taken to fix it will be a lot longer than the link time penalty)

https://reviews.llvm.org/D87121#2260177 I don't use it as a motivation for the patch. I created this patch because I think it is the right thing to do.

>> You can save 2 or 3 lines by not making this change, but to me the "mental complexity" is larger: a lock is needed to prevent interleaving output from multiple threads.
>
> There's still locking though, so I'm not sure it's a huge difference in mental complexity - locking on the write, to locking on a 10 line function that writes a few times?

I keep the lock as I don't want to take the risk in this change. The mental complexity is different.

Before:  (a) the coder worries that the raw_ostream::operator<< may access data potentially concurrently modified; (b): concurrent ErrorHandler::log/warn/error may interleave their output so a lock is necessary
Now: only (a) exists


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87272