vsapsai created this revision.
vsapsai added reviewers: bruno, dexonsmith.
Herald added subscribers: cfe-commits, ributzka, jkorous.
Herald added a project: clang.
Partially reverts 0a2be46cfdb698fefcc860a56b47dde0884d5335 as it turned
out to cause redundant module rebuilds in multi-process incremental builds.
When a module was getting out of date, all compilation processes started at the
same time were marking it as `ToBuild`. So each process was building the same
module instead of checking if it was built by someone else and using that
result. In addition to the work duplication, contention on the same .pcm file
wasn't making builds faster.
Note that for a single-process build this change would cause redundant module
reads and validations. But reading a module is faster than building it and
multi-process builds are more common than single-process. So I'm willing to
make such a trade-off.
rdar://problem/54395127
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72860
Files:
clang/include/clang/Serialization/InMemoryModuleCache.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/InMemoryModuleCache.cpp
clang/lib/Serialization/ModuleManager.cpp
clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
clang/test/Modules/implicit-invalidate-chain.c
clang/unittests/Frontend/FrontendActionTest.cpp
clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
Index: clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
===================================================================
--- clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
+++ clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
@@ -24,12 +24,11 @@
TEST(InMemoryModuleCacheTest, initialState) {
InMemoryModuleCache Cache;
- EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
+ EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
EXPECT_FALSE(Cache.isPCMFinal("B"));
- EXPECT_FALSE(Cache.shouldBuildPCM("B"));
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
- EXPECT_DEATH(Cache.tryToDropPCM("B"), "PCM to remove is unknown");
+ EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
#endif
}
@@ -40,37 +39,33 @@
InMemoryModuleCache Cache;
EXPECT_EQ(RawB, &Cache.addPCM("B", std::move(B)));
- EXPECT_EQ(InMemoryModuleCache::Tentative, Cache.getPCMState("B"));
EXPECT_EQ(RawB, Cache.lookupPCM("B"));
EXPECT_FALSE(Cache.isPCMFinal("B"));
- EXPECT_FALSE(Cache.shouldBuildPCM("B"));
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
- EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
- "Trying to override tentative PCM");
+ EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
+ "Already has a non-final PCM");
#endif
}
-TEST(InMemoryModuleCacheTest, addBuiltPCM) {
+TEST(InMemoryModuleCacheTest, addFinalPCM) {
auto B = getBuffer(1);
auto *RawB = B.get();
InMemoryModuleCache Cache;
- EXPECT_EQ(RawB, &Cache.addBuiltPCM("B", std::move(B)));
- EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B"));
+ EXPECT_EQ(RawB, &Cache.addFinalPCM("B", std::move(B)));
EXPECT_EQ(RawB, Cache.lookupPCM("B"));
EXPECT_TRUE(Cache.isPCMFinal("B"));
- EXPECT_FALSE(Cache.shouldBuildPCM("B"));
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
- EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
+ EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
"Trying to override finalized PCM");
#endif
}
-TEST(InMemoryModuleCacheTest, tryToDropPCM) {
+TEST(InMemoryModuleCacheTest, tryToRemovePCM) {
auto B1 = getBuffer(1);
auto B2 = getBuffer(2);
auto *RawB1 = B1.get();
@@ -78,27 +73,22 @@
ASSERT_NE(RawB1, RawB2);
InMemoryModuleCache Cache;
- EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
EXPECT_EQ(RawB1, &Cache.addPCM("B", std::move(B1)));
- EXPECT_FALSE(Cache.tryToDropPCM("B"));
+ EXPECT_FALSE(Cache.tryToRemovePCM("B"));
EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
- EXPECT_EQ(InMemoryModuleCache::ToBuild, Cache.getPCMState("B"));
EXPECT_FALSE(Cache.isPCMFinal("B"));
- EXPECT_TRUE(Cache.shouldBuildPCM("B"));
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
- EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
- EXPECT_DEATH(Cache.tryToDropPCM("B"),
- "PCM to remove is scheduled to be built");
- EXPECT_DEATH(Cache.finalizePCM("B"), "Trying to finalize a dropped PCM");
+ EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
+ EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
#endif
// Add a new one.
- EXPECT_EQ(RawB2, &Cache.addBuiltPCM("B", std::move(B2)));
+ EXPECT_EQ(RawB2, &Cache.addFinalPCM("B", std::move(B2)));
EXPECT_TRUE(Cache.isPCMFinal("B"));
// Can try to drop again, but this should error and do nothing.
- EXPECT_TRUE(Cache.tryToDropPCM("B"));
+ EXPECT_TRUE(Cache.tryToRemovePCM("B"));
EXPECT_EQ(RawB2, Cache.lookupPCM("B"));
}
@@ -107,12 +97,10 @@
auto *RawB = B.get();
InMemoryModuleCache Cache;
- EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
EXPECT_EQ(RawB, &Cache.addPCM("B", std::move(B)));
// Call finalize.
Cache.finalizePCM("B");
- EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B"));
EXPECT_TRUE(Cache.isPCMFinal("B"));
}
Index: clang/unittests/Frontend/FrontendActionTest.cpp
===================================================================
--- clang/unittests/Frontend/FrontendActionTest.cpp
+++ clang/unittests/Frontend/FrontendActionTest.cpp
@@ -284,11 +284,9 @@
// Check whether the PCH was cached.
if (ShouldCache)
- EXPECT_EQ(InMemoryModuleCache::Final,
- Compiler.getModuleCache().getPCMState(PCHFilename));
+ EXPECT_TRUE(Compiler.getModuleCache().isPCMFinal(PCHFilename));
else
- EXPECT_EQ(InMemoryModuleCache::Unknown,
- Compiler.getModuleCache().getPCMState(PCHFilename));
+ EXPECT_EQ(nullptr, Compiler.getModuleCache().lookupPCM(PCHFilename));
}
}
Index: clang/test/Modules/implicit-invalidate-chain.c
===================================================================
--- clang/test/Modules/implicit-invalidate-chain.c
+++ /dev/null
@@ -1,67 +0,0 @@
-// RUN: rm -rf %t1 %t2 %t-include
-// RUN: mkdir %t-include
-// RUN: echo 'module D { header "D.h" }' >> %t-include/module.modulemap
-
-// Run with -verify, which onliy gets remarks from the main TU.
-//
-// RUN: echo '#define D 0' > %t-include/D.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t1 \
-// RUN: -fdisable-module-hash -fsyntax-only \
-// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \
-// RUN: -Rmodule-build -Rmodule-import %s
-// RUN: echo '#define D 11' > %t-include/D.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t1 \
-// RUN: -fdisable-module-hash -fsyntax-only \
-// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \
-// RUN: -Rmodule-build -Rmodule-import -verify %s
-
-// Run again, using FileCheck to check remarks from the module builds. This is
-// the key test: after the first attempt to import an out-of-date 'D', all the
-// modules have been invalidated and are not imported again until they are
-// rebuilt.
-//
-// RUN: echo '#define D 0' > %t-include/D.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t2 \
-// RUN: -fdisable-module-hash -fsyntax-only \
-// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \
-// RUN: -Rmodule-build -Rmodule-import %s
-// RUN: echo '#define D 11' > %t-include/D.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t2 \
-// RUN: -fdisable-module-hash -fsyntax-only \
-// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \
-// RUN: -Rmodule-build -Rmodule-import %s 2>&1 |\
-// RUN: FileCheck %s -implicit-check-not "remark:"
-
-#include "A.h" // \
- expected-remark-re{{importing module 'A' from '{{.*[/\\]}}A.pcm'}} \
- expected-remark-re{{importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'}} \
- expected-remark-re{{importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'}} \
- expected-remark-re{{importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'}} \
- expected-remark-re{{building module 'A' as '{{.*[/\\]}}A.pcm'}} \
- expected-remark{{finished building module 'A'}} \
- expected-remark-re{{importing module 'A' from '{{.*[/\\]}}A.pcm'}} \
- expected-remark-re{{importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'}} \
- expected-remark-re{{importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'}} \
- expected-remark-re{{importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'}}
-// CHECK: remark: importing module 'A' from '{{.*[/\\]}}A.pcm'
-// CHECK: remark: importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'
-// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'
-// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'
-// CHECK: remark: building module 'A'
-// CHECK: remark: building module 'B'
-// CHECK: remark: building module 'C'
-// CHECK: remark: building module 'D'
-// CHECK: remark: finished building module 'D'
-// CHECK: remark: importing module 'D' from '{{.*[/\\]}}D.pcm'
-// CHECK: remark: finished building module 'C'
-// CHECK: remark: importing module 'C' from '{{.*[/\\]}}C.pcm'
-// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'
-// CHECK: remark: finished building module 'B'
-// CHECK: remark: importing module 'B' from '{{.*[/\\]}}B.pcm'
-// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'
-// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'
-// CHECK: remark: finished building module 'A'
-// CHECK: remark: importing module 'A' from '{{.*[/\\]}}A.pcm'
-// CHECK: remark: importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'
-// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'
-// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'
Index: clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
===================================================================
--- clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
+++ /dev/null
@@ -1,3 +0,0 @@
-module A { header "A.h" }
-module B { header "B.h" }
-module C { header "C.h" }
Index: clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
===================================================================
--- clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
+++ /dev/null
@@ -1,2 +0,0 @@
-// C
-#include "D.h"
Index: clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
===================================================================
--- clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
+++ /dev/null
@@ -1,2 +0,0 @@
-// B
-#include "C.h"
Index: clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
===================================================================
--- clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
+++ /dev/null
@@ -1,2 +0,0 @@
-// A
-#include "B.h"
Index: clang/lib/Serialization/ModuleManager.cpp
===================================================================
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -163,7 +163,7 @@
// Load the contents of the module
if (std::unique_ptr<llvm::MemoryBuffer> Buffer = lookupBuffer(FileName)) {
// The buffer was already provided for us.
- NewModule->Buffer = &ModuleCache->addBuiltPCM(FileName, std::move(Buffer));
+ NewModule->Buffer = &ModuleCache->addFinalPCM(FileName, std::move(Buffer));
// Since the cached buffer is reused, it is safe to close the file
// descriptor that was opened while stat()ing the PCM in
// lookupModuleFile() above, it won't be needed any longer.
@@ -173,11 +173,6 @@
NewModule->Buffer = Buffer;
// As above, the file descriptor is no longer needed.
Entry->closeFile();
- } else if (getModuleCache().shouldBuildPCM(FileName)) {
- // Report that the module is out of date, since we tried (and failed) to
- // import it earlier.
- Entry->closeFile();
- return OutOfDate;
} else {
// Open the AST file.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buf((std::error_code()));
@@ -185,7 +180,7 @@
Buf = llvm::MemoryBuffer::getSTDIN();
} else {
// Get a buffer of the file and close the file descriptor when done.
- Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+ Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
}
if (!Buf) {
Index: clang/lib/Serialization/InMemoryModuleCache.cpp
===================================================================
--- clang/lib/Serialization/InMemoryModuleCache.cpp
+++ clang/lib/Serialization/InMemoryModuleCache.cpp
@@ -11,16 +11,6 @@
using namespace clang;
-InMemoryModuleCache::State
-InMemoryModuleCache::getPCMState(llvm::StringRef Filename) const {
- auto I = PCMs.find(Filename);
- if (I == PCMs.end())
- return Unknown;
- if (I->second.IsFinal)
- return Final;
- return I->second.Buffer ? Tentative : ToBuild;
-}
-
llvm::MemoryBuffer &
InMemoryModuleCache::addPCM(llvm::StringRef Filename,
std::unique_ptr<llvm::MemoryBuffer> Buffer) {
@@ -30,11 +20,11 @@
}
llvm::MemoryBuffer &
-InMemoryModuleCache::addBuiltPCM(llvm::StringRef Filename,
+InMemoryModuleCache::addFinalPCM(llvm::StringRef Filename,
std::unique_ptr<llvm::MemoryBuffer> Buffer) {
auto &PCM = PCMs[Filename];
assert(!PCM.IsFinal && "Trying to override finalized PCM?");
- assert(!PCM.Buffer && "Trying to override tentative PCM?");
+ assert(!PCM.Buffer && "Already has a non-final PCM");
PCM.Buffer = std::move(Buffer);
PCM.IsFinal = true;
return *PCM.Buffer;
@@ -49,24 +39,21 @@
}
bool InMemoryModuleCache::isPCMFinal(llvm::StringRef Filename) const {
- return getPCMState(Filename) == Final;
-}
-
-bool InMemoryModuleCache::shouldBuildPCM(llvm::StringRef Filename) const {
- return getPCMState(Filename) == ToBuild;
+ auto I = PCMs.find(Filename);
+ if (I == PCMs.end())
+ return false;
+ return I->second.IsFinal;
}
-bool InMemoryModuleCache::tryToDropPCM(llvm::StringRef Filename) {
+bool InMemoryModuleCache::tryToRemovePCM(llvm::StringRef Filename) {
auto I = PCMs.find(Filename);
assert(I != PCMs.end() && "PCM to remove is unknown...");
auto &PCM = I->second;
- assert(PCM.Buffer && "PCM to remove is scheduled to be built...");
-
if (PCM.IsFinal)
return true;
- PCM.Buffer.reset();
+ PCMs.erase(I);
return false;
}
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4304,7 +4304,7 @@
WritingAST = false;
if (ShouldCacheASTInMemory) {
// Construct MemoryBuffer and update buffer manager.
- ModuleCache.addBuiltPCM(OutputFile,
+ ModuleCache.addFinalPCM(OutputFile,
llvm::MemoryBuffer::getMemBufferCopy(
StringRef(Buffer.begin(), Buffer.size())));
}
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4502,7 +4502,7 @@
if (ShouldFinalizePCM)
MC.finalizePCM(FileName);
else
- MC.tryToDropPCM(FileName);
+ MC.tryToRemovePCM(FileName);
});
ModuleFile &F = *M;
BitstreamCursor &Stream = F.Stream;
Index: clang/include/clang/Serialization/InMemoryModuleCache.h
===================================================================
--- clang/include/clang/Serialization/InMemoryModuleCache.h
+++ clang/include/clang/Serialization/InMemoryModuleCache.h
@@ -45,61 +45,35 @@
llvm::StringMap<PCM> PCMs;
public:
- /// There are four states for a PCM. It must monotonically increase.
- ///
- /// 1. Unknown: the PCM has neither been read from disk nor built.
- /// 2. Tentative: the PCM has been read from disk but not yet imported or
- /// built. It might work.
- /// 3. ToBuild: the PCM read from disk did not work but a new one has not
- /// been built yet.
- /// 4. Final: indicating that the current PCM was either built in this
- /// process or has been successfully imported.
- enum State { Unknown, Tentative, ToBuild, Final };
-
- /// Get the state of the PCM.
- State getPCMState(llvm::StringRef Filename) const;
-
/// Store the PCM under the Filename.
///
- /// \pre state is Unknown
- /// \post state is Tentative
+ /// \pre PCM for the same Filename shouldn't be in cache already.
/// \return a reference to the buffer as a convenience.
llvm::MemoryBuffer &addPCM(llvm::StringRef Filename,
std::unique_ptr<llvm::MemoryBuffer> Buffer);
- /// Store a just-built PCM under the Filename.
+ /// Store a final PCM under the Filename.
///
- /// \pre state is Unknown or ToBuild.
- /// \pre state is not Tentative.
+ /// \pre PCM for the same Filename shouldn't be in cache already.
/// \return a reference to the buffer as a convenience.
- llvm::MemoryBuffer &addBuiltPCM(llvm::StringRef Filename,
+ llvm::MemoryBuffer &addFinalPCM(llvm::StringRef Filename,
std::unique_ptr<llvm::MemoryBuffer> Buffer);
- /// Try to remove a buffer from the cache. No effect if state is Final.
+ /// Try to remove a PCM from the cache. No effect if it is Final.
///
- /// \pre state is Tentative/Final.
- /// \post Tentative => ToBuild or Final => Final.
- /// \return false on success, i.e. if Tentative => ToBuild.
- bool tryToDropPCM(llvm::StringRef Filename);
+ /// \return false on success.
+ bool tryToRemovePCM(llvm::StringRef Filename);
/// Mark a PCM as final.
- ///
- /// \pre state is Tentative or Final.
- /// \post state is Final.
void finalizePCM(llvm::StringRef Filename);
- /// Get a pointer to the pCM if it exists; else nullptr.
+ /// Get a pointer to the PCM if it exists; else nullptr.
llvm::MemoryBuffer *lookupPCM(llvm::StringRef Filename) const;
/// Check whether the PCM is final and has been shown to work.
///
/// \return true iff state is Final.
bool isPCMFinal(llvm::StringRef Filename) const;
-
- /// Check whether the PCM is waiting to be built.
- ///
- /// \return true iff state is ToBuild.
- bool shouldBuildPCM(llvm::StringRef Filename) const;
};
} // end namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
