[PATCH] D36492: [time-report] Add preprocessor timer

Matthias Braun via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 22 13:14:10 PDT 2017

MatzeB added inline comments.

Comment at: lib/Lex/Preprocessor.cpp:746
 void Preprocessor::Lex(Token &Result) {
+  llvm::TimeRegion(PPOpts->getTimer());
modocache wrote:
> erik.pilkington wrote:
> > Doesn't this just start a timer and immediately end the timer? Shouldn't we do: `llvm::TimeRegion LexTime(PPOpts->getTimer())` so that the dtor gets called when this function returns and we track the time spent in this function?
> > 
> > Also: this is a pretty hot function, and it looks like TimeRegion does some non-trivial work if time is being tracked. Have you tried testing this on a big c++ file with and without this patch and seeing what the difference in compile time looks like?
> Ah, yes you're right! Sorry about that. Actually keeping the timer alive for the duration of the method also reveals that the method is called recursively, which causes an assert, because timers can't be started twice.
> Another spot in Clang works around this with a "reference counted" timer: https://github.com/llvm-mirror/clang/blob/6ac9c51ede0a50cca13dd4ac03562c036f7a3f48/lib/CodeGen/CodeGenAction.cpp#L130-L134. I have a more generic version of this "reference counting timer" that I've been using for some of the other timers I've been adding; maybe I'll use it here as well.
FWIF: I share Eriks concerns about compiletime. Timers are enabled in optimized builds, and querying them is not free. So putting one into a function that is called a lot and is time critical seems like a bad idea (do benchmarking to prove or disprove this!).


More information about the cfe-commits mailing list