[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc

[Daniel Jasper via Phabricator via cfe-commits]

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:
> 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?


https://reviews.llvm.org/D28207