[PATCH] D112915: [clang][modules] Track number of includes per submodule

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Implementation looks a lot cleaner!

I'd still like to drop NumIncludes first if possible because I think it'll be easier to reason about without this extra layer of complexity. Also, that'd mitigate the potential regression in `.pcm` size.

(Note: I'd be more comfortable with @vsapsai and/or @rsmith reviewing the overall direction; I just jumped in for the implementation details.)



================
Comment at: clang/include/clang/Lex/HeaderSearch.h:133-139
-
-  /// Determine whether this is a non-default header file info, e.g.,
-  /// it corresponds to an actual header we've included or tried to include.
-  bool isNonDefault() const {
-    return isImport || isPragmaOnce || NumIncludes || ControllingMacro ||
-      ControllingMacroID;
-  }
----------------
Looks like this is already dead code? If so, please separate out and commit ahead of time (e.g., now).


================
Comment at: clang/include/clang/Serialization/ModuleFile.h:399
+  /// include information.
+  llvm::StringMap<llvm::SmallVector<uint64_t, 64>> SubmoduleIncludedFiles;
+
----------------
Each StringMapEntry is going to have a pretty big allocation here, for a 512B vector. Given that it doesn't need to be after creation, it'd be more efficient to use this pattern:
```
lang=c++
llvm::StringMap<ArrayRef<uint64_t>> SubmoduleIncludedFiles;
SpecificBumpPtrAlloc<uint64_t> SubmoduleIncludedFilesAlloc;

// later
MutableArrayRef<uint64_t> Files(SubmoduleIncludedFiles.Allocate(Record.size()), Record.size());
llvm::copy(Record, Files.begin());
SubmoduleIncludedFiles[Key] = Files;
```

That said, I feel like this should just be:
```
lang=c++
DenseMap<StringRef, StringRef> SubmoduleIncludedFiles;
```
The key can point at the name of the submodule, which must already exist somewhere without needing a StringMap to create a new copy of it. The value is a lazily deserialized blob.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:1323
+        auto &ModNumIncludes = SubmoduleIncludeStates[M].NumIncludes;
+        for (unsigned UID = 0; UID <= ModNumIncludes.maxUID(); ++UID) {
+          CurSubmoduleIncludeState->NumIncludes[UID] += ModNumIncludes[UID];
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > Iterating over all FileEntries is probably not very efficient, as Volodymyr mentioned. Thinking about how to make this more efficient...
> > My suggestion above to drop FileEntryMap in favour of a simple DenseMap would help a bit, just iterating through the files actually included by the submodules.
> > 
> > Further, I wonder if "num-includes"/file/submodule (`unsigned`) is actually useful, vs. "was-included"/file/submodule (`bool`). The only observer I see is `HeaderSearch::PrintStats()` but maybe I missed something?
> > 
> > If I'm right and we can switch to `bool`, then NumIncludes becomes a `DenseSet<FileEntry *> IncludedFiles` (or `DenseSet<unsigned>` for UIDs). (BTW, I also wondered if you could rotate the map, using File as the outer key, and then use bitsets for the sbumodules, but I doubt this is better, since most files are only included by a few submodules, right?)
> > 
> > Then you can just do a set union here. Also simplifies bitcode serialization.
> > 
> > (If a `bool`/set is sufficient, then I'd suggest landing that first/separately, before adding the per-submodule granularity in this patch.)
> For each file, we need to have three distinct states: not included at all, included exactly once (`firstTimeLexingFile`), included more than once. This means we can't use a single `DenseSet`.
> 
> But we could use a `DenseMap<Key, bool>`, where "not included at all" can be expressed as being absent from the map, exactly once as having `true` in the map and more than once as having `false` in the map. Alternatively, we could use two `DenseSet` instances to encode the same, but I don't think having two lookups per file to determine stuff is efficient.
> 
> I can look into this in a follow-up patch.
Seems like a DenseSet could still be used by having HeaderInfo pass back the WasInserted bit from the insertion to the preprocessor, and threading it through to Preprocessor::HandleEndOfFile (the only caller of FirstTimeLexingFile):
```
lang=c++
bool IsFirst = Set.insert(Key).second;
```
The threading doesn't seem too hard. Looking at main:
- Preprocessor::HandleHeaderIncludeOrImport calls HeaderInfo::ShouldEnterIncludeFile. This does the `++FI.NumIncludes` (going from 0 to 1). Instead, it could be `IsFirst = !FI.WasIncluded; FI.WasIncluded = true;`, then return `IsFirst` somehow. (Then your patch can pull `IsFirst` from the `insert().second`).
- Preprocessor::HandleHeaderIncludeOrImport calls Preprocessor::EnterSourceFile. This creates a new Lexer for that file. `IsFirst` can be stored on that Lexer.
- Preprocessor::HandleEndOfFile calls FirstTimeLexingFile. Instead, it can check the new accessor `CurLexer->isFirstTimeLexing()`.

> I can look into this in a follow-up patch.

Follow-up might be okay, but it'd be nice to remove an axis of complexity before adding a new one if it's reasonable. E.g., it'll be easier to debug emergent issues from changing it to a simple set since there's less machinery to worry about.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:5720-5722
+    case SUBMODULE_NUM_INCLUDES:
+      F.SubmoduleIncludedFiles.insert(
+          {CurrentModule->getFullModuleName(), Record});
----------------
This looks lazy, but a bunch of work was just done to decode the `Record` from bitcode. To make this actually lazy, you can encode the data in a blob, which doesn't have to be decoded until it's used.



================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2173-2177
+void ASTWriter::addIncludedFiles(
+    const llvm::DenseMap<const FileEntry *, unsigned> &Files,
+    RecordDataImpl &Record) {
+  // Map the FileEntry map into an input file ID vector.
+  llvm::SmallVector<std::pair<uint64_t, unsigned>> FileIDs;
----------------
Why does the count need to be encoded?

The only observer is `Preprocessor::HandleEndOfFile`. If it gets called again for this file, it'll be after `++NumIncludes`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915