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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 5 inline comments as done.
jansvoboda11 added a comment.

In D112915#3122340 <https://reviews.llvm.org/D112915#3122340>, @dexonsmith wrote:

> 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.

Done in D114096 <https://reviews.llvm.org/D114096>. Thanks for the feedback.



================
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;
-  }
----------------
dexonsmith wrote:
> Looks like this is already dead code? If so, please separate out and commit ahead of time (e.g., now).
Done in D114092.


================
Comment at: clang/include/clang/Serialization/ModuleFile.h:399
+  /// include information.
+  llvm::StringMap<llvm::SmallVector<uint64_t, 64>> SubmoduleIncludedFiles;
+
----------------
dexonsmith wrote:
> 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.
I switched to `StringRef` value in the latest revision. Unfortunately, had to use `std::string` as the key instead of `StringRef`, since `getFullModuleName` constructs the string on heap. That forced me to use `std::map`, too. I'll explore using something else entirely as the key.


================
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];
----------------
dexonsmith wrote:
> 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.
Extracted into D114093.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915