djasper added inline comments.
================ Comment at: lib/Basic/Diagnostic.cpp:179 + + // 2nd most frequent case: L is before the first diag state change. + FullSourceLoc FirstStateChangePos = DiagStatePoints[1].Loc; ---------------- rsmith wrote: > djasper wrote: > > rsmith wrote: > > > It's surprising to me that this would be particularly frequent. > > > > > > I suspect what you're actually seeing is a consequence of a bug in how we > > > manage `DiagStatePoint`s with modules. It looks like > > > `ASTReader::InitializeContext` is called once per top-level PCM file that > > > we load, and its call to `ReadPragmaDiagnosticMappings` adds entries to > > > the `DiagStatePoints` list regardless of whether they've already been > > > added. So, we'll end up with duplicates in the `DiagStatePoints` list, > > > and it won't even be in translation unit order. > > > > > > Can you take a look at the `DiagStatePoints` list for a translation unit > > > where you see a performance problem and check whether it seems reasonable? > > I looked at the DiagStatePoints and they do look somewhat sane but > > suspicious. > > > > The translation unit I am looking at has (AFAICT) only includes that get > > translated to modules. DiagStatePoints only has entries for the translation > > unit itself, not a single one coming from any of the modules. So > > DiagStatePoints looks like: > > > > [0]: <<invalid>> (for the command line) > > [1]: myfile.cc:100 > > [2]: myfile.cc:100 > > ... > > [2500]: myfile.cc:2800 > > > > And because of that, the new fast path is hit every single time when a > > source location coming from a module is queried. There are always two > > entries for a line of myfile.cc which always seem to denote entering and > > exiting a macro. > > > > So, specific questions: > > - Should there by DiagStatePoints from modules? > > - Should there really be a DiagStatePoint entry (or actually two) for every > > macro invocation in myfile.cc? > Yes, there should be `DiagStatePoints` from modules, but support for that > seems to basically be unimplemented at this point. > > I believe the `DiagStatePoints` for macros are coming from a `_Pragma("clang > diagnostic ...")` within the macro expansion. Doesn't look like there's any > other way we'd create them. Ah yes, you are right. I can probably fix that and I presume then this patch won't help as much anymore. https://reviews.llvm.org/D28207 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits