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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
