Skip to content

Commit

Permalink
[Serialization] Do less redundant work computing affecting module maps (
Browse files Browse the repository at this point in the history
#66933)

We're traversing the same chains of module ancestors and include
locations repeatedly, despite already populating sets that can detect
it!

This is a problem because translateFile() is expensive. I think we can
avoid it entirely, but this seems like an improvement either way.

I removed a callback indirection rather than giving it a more
complicated
signature, and accordingly renamed the lambdas to be more concrete.
  • Loading branch information
sam-mccall committed Sep 22, 2023
1 parent efd5cde commit 0f05096
Showing 1 changed file with 24 additions and 23 deletions.
47 changes: 24 additions & 23 deletions clang/lib/Serialization/ASTWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,6 @@ namespace {

std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
Module *RootModule) {
std::set<const FileEntry *> ModuleMaps{};
std::set<const Module *> ProcessedModules;
SmallVector<const Module *> ModulesToProcess{RootModule};

const HeaderSearch &HS = PP.getHeaderSearchInfo();
Expand Down Expand Up @@ -195,42 +193,45 @@ std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
const ModuleMap &MM = HS.getModuleMap();
SourceManager &SourceMgr = PP.getSourceManager();

auto ForIncludeChain = [&](FileEntryRef F,
llvm::function_ref<void(FileEntryRef)> CB) {
CB(F);
std::set<const FileEntry *> ModuleMaps{};
auto CollectIncludingModuleMaps = [&](FileEntryRef F) {
if (!ModuleMaps.insert(F).second)
return;
FileID FID = SourceMgr.translateFile(F);
SourceLocation Loc = SourceMgr.getIncludeLoc(FID);
// The include location of inferred module maps can point into the header
// file that triggered the inferring. Cut off the walk if that's the case.
while (Loc.isValid() && isModuleMap(SourceMgr.getFileCharacteristic(Loc))) {
FID = SourceMgr.getFileID(Loc);
CB(*SourceMgr.getFileEntryRefForID(FID));
if (!ModuleMaps.insert(*SourceMgr.getFileEntryRefForID(FID)).second)
break;
Loc = SourceMgr.getIncludeLoc(FID);
}
};

auto ProcessModuleOnce = [&](const Module *M) {
for (const Module *Mod = M; Mod; Mod = Mod->Parent)
if (ProcessedModules.insert(Mod).second) {
auto Insert = [&](FileEntryRef F) { ModuleMaps.insert(F); };
// The containing module map is affecting, because it's being pointed
// into by Module::DefinitionLoc.
if (auto ModuleMapFile = MM.getContainingModuleMapFile(Mod))
ForIncludeChain(*ModuleMapFile, Insert);
// For inferred modules, the module map that allowed inferring is not in
// the include chain of the virtual containing module map file. It did
// affect the compilation, though.
if (auto ModuleMapFile = MM.getModuleMapFileForUniquing(Mod))
ForIncludeChain(*ModuleMapFile, Insert);
}
std::set<const Module *> ProcessedModules;
auto CollectIncludingMapsFromAncestors = [&](const Module *M) {
for (const Module *Mod = M; Mod; Mod = Mod->Parent) {
if (!ProcessedModules.insert(Mod).second)
break;
// The containing module map is affecting, because it's being pointed
// into by Module::DefinitionLoc.
if (auto ModuleMapFile = MM.getContainingModuleMapFile(Mod))
CollectIncludingModuleMaps(*ModuleMapFile);
// For inferred modules, the module map that allowed inferring is not in
// the include chain of the virtual containing module map file. It did
// affect the compilation, though.
if (auto ModuleMapFile = MM.getModuleMapFileForUniquing(Mod))
CollectIncludingModuleMaps(*ModuleMapFile);
}
};

for (const Module *CurrentModule : ModulesToProcess) {
ProcessModuleOnce(CurrentModule);
CollectIncludingMapsFromAncestors(CurrentModule);
for (const Module *ImportedModule : CurrentModule->Imports)
ProcessModuleOnce(ImportedModule);
CollectIncludingMapsFromAncestors(ImportedModule);
for (const Module *UndeclaredModule : CurrentModule->UndeclaredUses)
ProcessModuleOnce(UndeclaredModule);
CollectIncludingMapsFromAncestors(UndeclaredModule);
}

return ModuleMaps;
Expand Down

0 comments on commit 0f05096

Please sign in to comment.