vsk added inline comments.
================ Comment at: lib/Lex/Preprocessor.cpp:746 void Preprocessor::Lex(Token &Result) { + llvm::TimeRegion(PPOpts->getTimer()); + ---------------- MatzeB wrote: > 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!). The timer is not started or queried unless -ftime-report is enabled. In the common case the overhead amounts to one extra null-check. And when we're collecting timing information, some performance degradation (say, within 5%) should be acceptable. I agree that we should get a sense for what the overhead is, but am not convinced that this should be a blocking issue. https://reviews.llvm.org/D36492 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits