[C++20] [Modules] Writing PCM will be unexpectedly slow with additional import.

[ChuanqiXu9]

We can find the reproducer at: https://discourse.llvm.org/t/modules-increased-build-times/68755/7

For short:

// test.cppm
module;

#include <iostream>

export module test;
import vulkan; // Vulcan is constructed big module.

will take 3~4s to generate the PCM.

And

module;
#include <iostream>
export module test.without.import;

will take about 0.5s to complete only.

Note that:

#include <iostream>
import vulkan;

will take 0.5s to complete too.

Currently I located in ASTWriter::WriteDecl. But I am not sure if this can be fixed simply.

[llvmbot]

@llvm/issue-subscribers-clang-modules

[rnikander]

@ChuanqiXu9 I may be hitting this. I've got some files taking around 45 seconds to compile and it's killing me. I wonder if there is a temporary workaround that I could apply to LLVM source locally, without the more well-thought-out fix that you will need to apply. I'm interested in compilers and hacking on Clang, so I could try it. Can you say any more about what you found in ASTWriter::WriteDecl that might be the problem?

[koplas]

It could help to upload a minimal reproducer and a flame graph. The most significant difference when importing a module is that clang::FindExternalVisibleDeclsByName gets called which seems to be slow with big modules.

@ChuanqiXu9 already linked a thread with further info: https://discourse.llvm.org/t/modules-increased-build-times/68755.
I only have files that take 5 seconds longer to compile and my reproducer is far from minimal, so a better reproducer could help.

[koplas]

@rnikander if you want to hack on it, you could try to modify:

llvm-project/clang/lib/Serialization/ASTWriterDecl.cpp

Lines 2490 to 2532 in e58a493

	/// isRequiredDecl - Check if this is a "required" Decl, which must be seen by
	/// consumers of the AST.
	///
	/// Such decls will always be deserialized from the AST file, so we would like
	/// this to be as restrictive as possible. Currently the predicate is driven by
	/// code generation requirements, if other clients have a different notion of
	/// what is "required" then we may have to consider an alternate scheme where
	/// clients can iterate over the top-level decls and get information on them,
	/// without necessary deserializing them. We could explicitly require such
	/// clients to use a separate API call to "realize" the decl. This should be
	/// relatively painless since they would presumably only do it for top-level
	/// decls.
	static bool isRequiredDecl(const Decl *D, ASTContext &Context,
	Module *WritingModule) {
	// Named modules have different semantics than header modules. Every named
	// module units owns a translation unit. So the importer of named modules
	// doesn't need to deserilize everything ahead of time.
	if (WritingModule && WritingModule->isModulePurview()) {
	// The PragmaCommentDecl and PragmaDetectMismatchDecl are MSVC's extension.
	// And the behavior of MSVC for such cases will leak this to the module
	// users. Given pragma is not a standard thing, the compiler has the space
	// to do their own decision. Let's follow MSVC here.
	if (isa<PragmaCommentDecl, PragmaDetectMismatchDecl>(D))
	return true;
	return false;
	}
	// An ObjCMethodDecl is never considered as "required" because its
	// implementation container always is.
	// File scoped assembly or obj-c or OMP declare target implementation must be
	// seen.
	if (isa<FileScopeAsmDecl, TopLevelStmtDecl, ObjCImplDecl>(D))
	return true;
	if (WritingModule && isPartOfPerModuleInitializer(D)) {
	// These declarations are part of the module initializer, and are emitted
	// if and when the module is imported, rather than being emitted eagerly.
	return false;
	}
	return Context.DeclMustBeEmitted(D);
	}

[ChuanqiXu9]

@rnikander if you want to hack on it, you could try to modify:

This might not be related to the problem.

@ChuanqiXu9 I may be hitting this. I've got some files taking around 45 seconds to compile and it's killing me. I wonder if there is a temporary workaround that I could apply to LLVM source locally, without the more well-thought-out fix that you will need to apply. I'm interested in compilers and hacking on Clang, so I could try it. Can you say any more about what you found in ASTWriter::WriteDecl that might be the problem?

I didn't find a quick solution/workaround for this. The direct cause for the problem is that for this code style:

// a.cppm
module;
#include <iostream>
export module;
import another; // another contains many decls <iostream>

When we merge the declarations from different modules, the declarations in this TU (a.cppm) will be the canonical declarations and the declarations from the imported modules will be the redeclarations. So we have to re-write the declarations in this TU. Also when we write declarations in this TU, we'll try to lookup the decls from the external sources (the imported modules), for example:

llvm-project/clang/lib/Serialization/ASTWriter.cpp

Lines 3960 to 3965 in edc8b60

	// Next we need to do a lookup with each name into this decl context to fully
	// populate any results from external sources. We don't actually use the
	// results of these lookups because we only want to use the results after all
	// results have been loaded and the pointers into them will be stable.
	for (auto &Name : Names)
	DC->lookup(Name);

This is what I found now. And as you see, it is pretty fundamental so I am not sure if there is a simple workaround.

[ChuanqiXu9]

As a dirty (and absolutely incorrect) verifier, you can insert the following code to:

llvm-project/clang/lib/Serialization/ASTWriter.cpp

Line 5578 in edc8b60

for (auto *RD : D->redecls()) {
      if (RD == D)
        continue;

      if (RD->isFromASTFile()) {
        DeclIDs[D] = RD->getGlobalID();
        return DeclIDs[D];
      }
}

Note that I don't think this is the correct direction. I think we need to do something in the lookup process if possible.

[ChuanqiXu9]

@rnikander and for the problem of slow compilation, I am wondering if some of the reason comes from the one phase compilation. For one phase compilation and two phase compilation, you can look this at: https://clang.llvm.org/docs/StandardCPlusPlusModules.html#how-to-produce-a-bmi and https://gitlab.kitware.com/cmake/cmake/-/issues/18355#note_1329192. And cmake choose to implement one phase model initially for the sake of compatibility. But two phase compilation may be faster locally. If the scale of your project isn't too large, you can try to implement the 2 phase compilation model by cmake scripts. Then we will have a better feeling for this.

[rnikander]

Okay, thank you. I'll dig around to try to understand better. I'd like to work on Clang but realistically I probably should focus on my projects.
So I guess this process of merging declarations from modules, which you mentioned, has to occur even with the declarations in the another module that are private to it, like the #include <iostream> in it's global module fragment?
I can't compile anything at the moment because main branch is segfaulting (another issue, someone already reported). I'll look again tomorrow.

[ChuanqiXu9]

I'd like to work on Clang but realistically I probably should focus on my projects.

Never mind. This is what open source is : )

So I guess this process of merging declarations from modules, which you mentioned, has to occur even with the declarations in the another module that are private to it, like the #include in it's global module fragment?

The deserialization and visibility checking are two related but different process. The things in global module fragment may be reachable so we have to deserialize them now.

[ChuanqiXu9]

I sent cf47e9f to mitigate the problem. The patch changes the writing BMI time of test.cppm from 5s to 3s in my environment. But 3s is still longer than I expected. So I'll keep this openning.

[ChuanqiXu9]

Note for developers: the root cause for the issue is that when we reconcile the declcontext redundantly many times. See the last section of ASTReader::loadDeclUpdateRecords. We will reconcile the decl context completely instead of by need, which may be a major reason for this issue.