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

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 added inline comments.


================
Comment at: include/clang/Lex/PreprocessorOptions.h:165
 public:
-  PreprocessorOptions() : UsePredefines(true), DetailedRecord(false),
+  PreprocessorOptions() : PPTimer("preprocessor", "Preprocessing"),
+                          UsePredefines(true),
----------------
modocache wrote:
> eduardo-elizondo wrote:
> > Should this be named "Lexing Time" or "Lexing" instead of "Preprocessing"?
> Good idea! Now that the timer's being started up in the `Preprocessor::Lex` method, it probably should be named "Lexing". Alternatively, I could move this into, say,` Lexer::Lex`. I guess there's not much of a distinction in Clang between "lexing" and "preprocessing."
> 
> I had originally picked this name because that's what appears in `gcc -ftime-report`, but I guess we don't need to keep the names the same.
Preprocessing does not means lexing. It includes much more, for example, file inclusion, include pathes elaboration, macros expansion, etc. We could merge all these parts or we could output the info for evey single piece. In any case I think we need a group of timers related to frontend (or maybe several groups: for pp, parser, Sema, codegen...). And maybe we need special switches to disable/enable the groups (-ftime-report could be used as some super switch for all of them). As result we need some repository of the timers and most probably this repository should be kept in PreprocessorOpts.

If there are no objections I can take this job on me and come up with new more general version of the patch. If it's OK then I'd like to collect the proposes about the required details of the time profile: what exactly we'd like to see in output? We have gcc output as an initial example but maybe we need more (or less) details?


================
Comment at: lib/Lex/Preprocessor.cpp:746
 void Preprocessor::Lex(Token &Result) {
+  llvm::TimeRegion(PPOpts->getTimer());
+
----------------
vsk wrote:
> 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.
Here we have more than "one extra null-check": we're dealing with constructor/destructor as well (and it's for every Lex()). I agree that it should be acceptable for us but we have to profile it (maybe we should not profile Lex() but something above it). 



https://reviews.llvm.org/D36492