[modules] Do not cache invalid state for modules that we attempted to load.
authorVolodymyr Sapsai <vsapsai@apple.com>
Fri, 17 Jan 2020 01:12:41 +0000 (17:12 -0800)
committerVolodymyr Sapsai <vsapsai@apple.com>
Fri, 17 Jan 2020 01:12:41 +0000 (17:12 -0800)
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

Reviewed By: dexonsmith

Differential Revision: https://reviews.llvm.org/D72860

12 files changed:
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 [deleted file]
clang/test/Modules/Inputs/implicit-invalidate-chain/B.h [deleted file]
clang/test/Modules/Inputs/implicit-invalidate-chain/C.h [deleted file]
clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap [deleted file]
clang/test/Modules/implicit-invalidate-chain.c [deleted file]
clang/unittests/Frontend/FrontendActionTest.cpp
clang/unittests/Serialization/InMemoryModuleCacheTest.cpp

index 7b5b5c1cf1be95bc4f286a654ee5ae3341075e67..c4814c40e1dfe5044fbbf1dd50a322d151f97f25 100644 (file)
@@ -45,61 +45,35 @@ class InMemoryModuleCache : public llvm::RefCountedBase<InMemoryModuleCache> {
   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
index 19e7ebe03a1fd3a17d6e8abc6db1d4ce986f5bf4..6497a4b06f7ad4d36ff187a7fe38f5bb7f7cb021 100644 (file)
@@ -4503,7 +4503,7 @@ ASTReader::ReadASTCore(StringRef FileName,
     if (ShouldFinalizePCM)
       MC.finalizePCM(FileName);
     else
-      MC.tryToDropPCM(FileName);
+      MC.tryToRemovePCM(FileName);
   });
   ModuleFile &F = *M;
   BitstreamCursor &Stream = F.Stream;
index 6eba48a1abe97548b760f1f9d4527a56c5e8c355..8247f5a84ecaabc122d5c87c6e9b6c2e91d19c93 100644 (file)
@@ -4304,7 +4304,7 @@ ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef,
   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 d35fa2a807f4dc33caa12f6c3594f6ca0d27d69d..68d941140a94a1e4f7d1ab9227fd47397f6c1f2f 100644 (file)
 
 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 @@ InMemoryModuleCache::addPCM(llvm::StringRef Filename,
 }
 
 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 @@ InMemoryModuleCache::lookupPCM(llvm::StringRef Filename) const {
 }
 
 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 daef502cdcb5e59c9a7b5cb275e1db6aed50033f..7406c8795fe498a8a3a6b90de843267fdcf6fd72 100644 (file)
@@ -163,7 +163,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   // 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 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     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,9 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
       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);
+      // The file is volatile because in a parallel build we expect multiple
+      // compiler processes to use the same module file rebuilding it if needed.
+      Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
     }
 
     if (!Buf) {
diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/A.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
deleted file mode 100644 (file)
index 2b9dab8..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-// A
-#include "B.h"
diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/B.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
deleted file mode 100644 (file)
index a2711d4..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-// B
-#include "C.h"
diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/C.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
deleted file mode 100644 (file)
index 231e35c..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-// C
-#include "D.h"
diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap b/clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
deleted file mode 100644 (file)
index f5b6360..0000000
+++ /dev/null
@@ -1,3 +0,0 @@
-module A { header "A.h" }
-module B { header "B.h" }
-module C { header "C.h" }
diff --git a/clang/test/Modules/implicit-invalidate-chain.c b/clang/test/Modules/implicit-invalidate-chain.c
deleted file mode 100644 (file)
index f7727a6..0000000
+++ /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 95be040ddf1b9306e59d40f2979f972c64bab318..d0a3020f446491d2f0c43a0ba18ca7c9fcba1d6d 100644 (file)
@@ -284,11 +284,9 @@ TEST(GeneratePCHFrontendAction, CacheGeneratedPCH) {
 
     // 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 ed5e1538eba74bda6bf65f246148c5e24f4c8901..7a5a3d4d2f5dd2ad877739bcde6c31fd1a91719e 100644 (file)
@@ -24,12 +24,11 @@ std::unique_ptr<MemoryBuffer> getBuffer(int I) {
 
 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 @@ TEST(InMemoryModuleCacheTest, addPCM) {
 
   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 @@ TEST(InMemoryModuleCacheTest, tryToDropPCM) {
   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 @@ TEST(InMemoryModuleCacheTest, finalizePCM) {
   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"));
 }