rsmith added a comment.

In, @mzolotukhin wrote:

> > Who is the audience for this information?
> >  What information do they want from a time report?
> >  How do we present that information in a way that's not misleading (given 
> > Clang's architecture)?
> I would find the timers extremely useful. Even if they overlap, they still 
> would 1) be a good indicator of a newly introduced problem and 2) give a 
> rough idea where frontend time is spent. I agree that it wouldn't be 
> super-accurate, but the numbers we usually operate are quite high (e.g. <some 
> part> started to take 1.5x time). When we decide to investigate it deeper, we 
> can use more accurate tools, such as profilers.

What kinds of <some part> would be useful to you? (How do you want the runtime 
of Clang broken down?) Are vertical slices (through all Clang's various layers 
and potentially parts of LLVM) -- as this patch will produce -- useful, or 
would you really want horizontal slices (as in, what part of Clang is actually 
spending the time)? Or just anything that's basically expected to be consistent 
from run to run, so you can identify that //something// has slowed down, even 
if you don't have enough information to really know what?

> All that said, if we can make timers more accurate/not-overlapping, that 
> surely would be helpful as well.

I think we need to fix the overlap issue as a prerequisite to adding timers 
with large amounts of overlap, especially self-overlap. Otherwise the numbers 
will likely do more harm than good, as they will significantly misattribute 
runtime. Fortunately, I think that should only require some relatively small 
changes to the LLVM timer infrastructure.

>> Can we deliver useful value compared to a modern, dedicated profiling tool?
> Having timers, especially producing results in the same format, as existing 
> LLVM timers, would be much more convenient than using profilers. With timers 
> I can simply add a compile-time flag to my test-suite run and get all the 
> numbers in the end of my usual run. With profilers it's a bit more 
> complicated.

Well, that depends on the profiler. With some profilers, you can just attach a 
profiler to your entire compilation and then ask it what the hottest functions 
were. But sure, if you have existing infrastructure built around 
`-ftime-report`, I can see that it makes sense to reuse that infrastructure.

> Speaking of timers overlapping and mutual calling: LLVM timers also have this 
> problem, and e.g. if there is problem is in some analysis (say, 
> ScalarEvolution), we see indications in several other passes using this 
> analysis (say, IndVarSimplify and LoopStrengthReduction). While it does not 
> immediately show the problematic spot, it gives you pretty strong hints to 
> where to look at first. So, having even not-perfect timers is still useful.

While LLVM may have some overlap between regions, the vertical timing slices 
are still pretty well aligned with the horizontal functionality slices. I 
expect the problems in Clang to be a //lot// worse. Suppose you enter N levels 
of parsing templates, and then trigger a whole bunch of template instantiation, 
AST deserialization, and code generation. Let's say that takes 1s in total. 
With this patch, I think we'd end up attributing Ns of compile time to "parsing 
templates", which is clearly very far from the truth, but will likely be listed 
as (by far) the slowest thing in the compilation. This does not even rise to 
the level of "not-perfect", it's going to be actively misleading in a lot of 
cases, and won't even necessarily point anywhere near the problematic spot.

I think with this patch and no fix to the overlap problem, we will find 
ourselves frequently fielding bugs where people say "X is slow" when it 
actually isn't. Plus I think we have the opportunity to deliver something 
that's hugely useful and not much harder to achieve (providing timing 
information that relates back to the source code), and I'd prefer we spend our 
complexity budget for this feature there.

> Also, I apologize for LGTMing the patch earlier while it was not properly 
> reviewed - I didn't notice it didn't have cfe-commits in subscribers, and it 
> had been waiting for a review for quite some time (so I assumed that all 
> interested parties had their chance to look at it).

No problem, these things happen :) And thank you for your feedback, it's very 

cfe-commits mailing list

Reply via email to