[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I tried implementing your suggestion (merging ranges of adjacent non-affecting files and avoiding `FileID` lookups), but the numbers from `-ftime-trace` are very noisy. I got more stable data by measuring clock cycles and instruction counts, but nothing conclusive yet.

Compilation of `CompilerInvocation.cpp` with implicit modules.

- previous approach with vector + `FileID` lookup: +0.64% cycles and +1.68% instructions,
- current approach with merged `SourceRange`s: +0.38% cycles and +1.11% instructions.

I'll post here as I experiment more and get more data.



================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5289-5290
+                ? NonAffectingInputs.end()
+                : llvm::lower_bound(NonAffectingInputs,
+                                    PP->getSourceManager().getFileID(Offset));
+  unsigned Idx = std::distance(NonAffectingInputs.begin(), It);
----------------
dexonsmith wrote:
> Why do you need to call `getFileID()` here?
> 
> Instead, I would expect this to be a search through a range of offsets (e.g., see my suggestion at https://reviews.llvm.org/D106876#3869247 -- `DroppedMMs` contains SourceLocations, not FileIDs).
> 
> Two benefits:
> 1. You don't need to call `getFileID()` to look up an offset.
> 2. You can merge adjacent non-affecting files (shrinking the search/storage significantly).
> 
> 
My reasoning was that if we search through a range of offsets, we're doing conceptually the same thing as `getFileID()` (which already has some optimizations baked in). Maybe the non-affecting files are indeed adjacent and we'll be able to merge most of them. I'll give it a shot and report back.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624