[PATCH] D86406: Speedup llvm-dwarfdump 3.9x

Tue Aug 25 00:09:32 PDT 2020

jhenderson added a comment.

In D86406#2234381 <https://reviews.llvm.org/D86406#2234381>, @JDevlieghere wrote:

> If I understand the patch correctly, the speedup is gained by caching the mode instead of trying to recompute it every time we call `colorsEnabled()`? This seems like a huge speedup for such a small thing, maybe I'm missing something?
>
> In D86406#2232912 <https://reviews.llvm.org/D86406#2232912>, @jhenderson wrote:
>
>> Also, perhaps this should be done by the stream itself - it seems like the stream knows whether or not it supports colours, so should we not just have the stream ignore attempts to use colours if the underlying thing doesn't support them?
>
> We already disable colors when they're not supported by the stream.

Maybe I misunderstood what this bit of the strace output was complaining about then:
`ioctl(1, TCGETS, 0x7ffd64d7f340)        = -1 ENOTTY (Inappropriate ioctl for device)`
(I have never used strace, so was only guessing based on its general appearance). I took it to mean that an attempt was made to change the colour, but failed because files don't support different colours, and that this failure is expensive.

> In D86406#2232918 <https://reviews.llvm.org/D86406#2232918>, @jankratochvil wrote:
>
>> OK, I agree, I will move it there.  That should also solve the caching problem.
>
> I don't think we should move it into the stream, if that's what you mean by "there". The color `cl::opt` is part of WithColor so we should check its value there.
>
> Can we achieve the same thing by resolving the ColorMode to `Enable` and `Disable` in the WihtColor constructor?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86406