Skip to content

Commit

Permalink
[clang][modules] Serialize Module::DefinitionLoc
Browse files Browse the repository at this point in the history
This is a prep patch for avoiding the quadratic number of calls to `HeaderSearch::lookupModule()` in `ASTReader` for each (transitively) loaded PCM file. (Specifically in the context of `clang-scan-deps`).

This patch explicitly serializes `Module::DefinitionLoc` so that we can stop relying on it being filled by the module map parser. This change also required change to the module map parser, where we used the absence of `DefinitionLoc` to determine whether a file came from a PCM file. We also need to make sure we consider the "containing" module map affecting when writing a PCM, so that it's not stripped during serialization, which ensures `DefinitionLoc` still ends up pointing to the correct offset. This is intended to be a NFC change.

Reviewed By: benlangmuir

Differential Revision: https://reviews.llvm.org/D150292
  • Loading branch information
jansvoboda11 committed Jul 17, 2023
1 parent cbc4bbb commit abcf7ce
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 12 deletions.
2 changes: 1 addition & 1 deletion clang/include/clang/Serialization/ASTBitCodes.h
Expand Up @@ -41,7 +41,7 @@ namespace serialization {
/// Version 4 of AST files also requires that the version control branch and
/// revision match exactly, since there is no backward compatibility of
/// AST files at this time.
const unsigned VERSION_MAJOR = 26;
const unsigned VERSION_MAJOR = 27;

/// AST file minor version number supported by this version of
/// Clang.
Expand Down
8 changes: 5 additions & 3 deletions clang/lib/Lex/ModuleMap.cpp
Expand Up @@ -2016,18 +2016,20 @@ void ModuleMapParser::parseModuleDecl() {
Module *ShadowingModule = nullptr;
if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) {
// We might see a (re)definition of a module that we already have a
// definition for in two cases:
// definition for in three cases:
// - If we loaded one definition from an AST file and we've just found a
// corresponding definition in a module map file, or
bool LoadedFromASTFile = Existing->DefinitionLoc.isInvalid();
bool LoadedFromASTFile = Existing->IsFromModuleFile;
// - If we previously inferred this module from different module map file.
bool Inferred = Existing->IsInferred;
// - If we're building a (preprocessed) module and we've just loaded the
// module map file from which it was created.
bool ParsedAsMainInput =
Map.LangOpts.getCompilingModule() == LangOptions::CMK_ModuleMap &&
Map.LangOpts.CurrentModule == ModuleName &&
SourceMgr.getDecomposedLoc(ModuleNameLoc).first !=
SourceMgr.getDecomposedLoc(Existing->DefinitionLoc).first;
if (!ActiveModule && (LoadedFromASTFile || ParsedAsMainInput)) {
if (!ActiveModule && (LoadedFromASTFile || Inferred || ParsedAsMainInput)) {
// Skip the module definition.
skipUntil(MMToken::RBrace);
if (Tok.is(MMToken::RBrace))
Expand Down
7 changes: 4 additions & 3 deletions clang/lib/Serialization/ASTReader.cpp
Expand Up @@ -5607,7 +5607,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
break;

case SUBMODULE_DEFINITION: {
if (Record.size() < 12)
if (Record.size() < 13)
return llvm::createStringError(std::errc::illegal_byte_sequence,
"malformed module definition");

Expand All @@ -5616,6 +5616,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
SubmoduleID GlobalID = getGlobalSubmoduleID(F, Record[Idx++]);
SubmoduleID Parent = getGlobalSubmoduleID(F, Record[Idx++]);
Module::ModuleKind Kind = (Module::ModuleKind)Record[Idx++];
SourceLocation DefinitionLoc = ReadSourceLocation(F, Record[Idx++]);
bool IsFramework = Record[Idx++];
bool IsExplicit = Record[Idx++];
bool IsSystem = Record[Idx++];
Expand All @@ -5636,8 +5637,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
ModMap.findOrCreateModule(Name, ParentModule, IsFramework, IsExplicit)
.first;

// FIXME: set the definition loc for CurrentModule, or call
// ModMap.setInferredModuleAllowedBy()
// FIXME: Call ModMap.setInferredModuleAllowedBy()

SubmoduleID GlobalIndex = GlobalID - NUM_PREDEF_SUBMODULE_IDS;
if (GlobalIndex >= SubmodulesLoaded.size() ||
Expand Down Expand Up @@ -5666,6 +5666,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
}

CurrentModule->Kind = Kind;
CurrentModule->DefinitionLoc = DefinitionLoc;
CurrentModule->Signature = F.Signature;
CurrentModule->IsFromModuleFile = true;
CurrentModule->IsSystem = IsSystem || CurrentModule->IsSystem;
Expand Down
24 changes: 19 additions & 5 deletions clang/lib/Serialization/ASTWriter.cpp
Expand Up @@ -200,7 +200,9 @@ std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
CB(F);
FileID FID = SourceMgr.translateFile(F);
SourceLocation Loc = SourceMgr.getIncludeLoc(FID);
while (Loc.isValid()) {
// 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));
Loc = SourceMgr.getIncludeLoc(FID);
Expand All @@ -209,11 +211,18 @@ std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,

auto ProcessModuleOnce = [&](const Module *M) {
for (const Module *Mod = M; Mod; Mod = Mod->Parent)
if (ProcessedModules.insert(Mod).second)
if (ProcessedModules.insert(Mod).second) {
auto Insert = [&](FileEntryRef F) { ModuleMaps.insert(F); };

This comment has been minimized.

Copy link
@zygoloid

zygoloid Sep 19, 2023

Collaborator

We're potentially doing a lot of repeated work here, if we visit the same module map multiple times in ForIncludeChain. We should change this lambda to return ModuleMaps.insert(F).second, and make ForIncludeChain stop if it returns false. Though I'm not sure that's related to the high performance cost of this change.

// 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, [&](FileEntryRef F) {
ModuleMaps.insert(F);
});
ForIncludeChain(*ModuleMapFile, Insert);
}
};

for (const Module *CurrentModule : ModulesToProcess) {
Expand Down Expand Up @@ -2687,6 +2696,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 4)); // Kind
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8)); // Definition location
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
Expand Down Expand Up @@ -2787,12 +2797,16 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
ParentID = SubmoduleIDs[Mod->Parent];
}

uint64_t DefinitionLoc =
SourceLocationEncoding::encode(getAdjustedLocation(Mod->DefinitionLoc));

// Emit the definition of the block.
{
RecordData::value_type Record[] = {SUBMODULE_DEFINITION,
ID,
ParentID,
(RecordData::value_type)Mod->Kind,
DefinitionLoc,
Mod->IsFramework,
Mod->IsExplicit,
Mod->IsSystem,
Expand Down

1 comment on commit abcf7ce

@sam-mccall
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this caused a large compile time regression for our module builds (seeing +100% wall, +20% CPU).

The CPU profiles show a new hot path collectNonAffectingInputFiles -> GetAffectingModuleMaps -> ProcessModuleOnce -> ForIncludeChain -> SourceManager::translateFile, which accounts for 20% CPU and was previously tiny (<0.5%).
At the bottom of that there are some calls to SourceManager::getLoadedSlocEntry, so I guess the extra wall time is IO loading SLocEntrys from imported modules.

I'm trying to understand exactly why the regression is so large. translateFile is an expensive function (linear search!) so calling it in a loop could explain it. However we should "only" be calling it twice as often as before this patch, unless the vast majority of our modules are inferred, which I don't think is the case.

Anyway, I'm going to do some more investigation over the next few days, just wanted to let you know in case you had ideas.
(Sorry to bring this up late - we've got compiler-release benchmarks but I think none for builds of large modules...)

Please sign in to comment.