Update for comments:

* Remove the support for multiple spanning layers
* Reorder renaming to blindly attempt a rename first, and only
  attempt to create the directory tree if that fails on the assumption
  that directory collisions are eventually going to be more likely than
  not.

Author: ojhunt

llvm/lib/CAS/OnDiskGraphDB.cpp

@@ -596,17 +596,15 @@ Error OnDiskGraphDB::TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
-  std::error_code RenameEC = sys::fs::create_directories(
-    sys::path::parent_path(Name.str())
-  );
-
-  // We consider both the creation of the directory for the persistent storage
-  // and the final rename itself to be a failure to rename. This is consistent
-  // with other filesystem APIs that provide file manipulation APIs that will
-  // automatically construct directories as needed, so it seems like
-  // a reasonable choice.
-  if (!RenameEC)
-    RenameEC = sys::fs::rename(TmpName, Name);
+  std::error_code RenameEC = sys::fs::rename(TmpName, Name);
+  if (RenameEC) {
+    // If the rename fails we assume we have not created all the required
+    // intermediate directories, so we'll try to do that now, and then
+    // retry the rename.
+    RenameEC = sys::fs::create_directories(sys::path::parent_path(Name.str()));
+    if (!RenameEC)
+      RenameEC = sys::fs::rename(TmpName, Name);
+  }
 
   if (Logger)
     Logger->log_TempFile_keep(TmpName, Name.str(), RenameEC);
@@ -1269,44 +1267,57 @@ void OnDiskGraphDB::getStandalonePath(StringRef Suffix, const IndexProxy &I,
   if (TempPath)
     TempPath->assign(RootPath.begin(), RootPath.end());
 
-  static const unsigned IntermediateDirCount = 2;
-  static const unsigned EntropyPerIntermediateDir = 8;
-  static const unsigned CharactersPerDirName =
-    ((EntropyPerIntermediateDir + 15) & ~7) / 8;
-
-  static const unsigned TotalEntropyBits =
-    IntermediateDirCount * EntropyPerIntermediateDir;
-  static const unsigned HashByteCount = ((TotalEntropyBits + 7) & ~7) / 8;
+  // 10 bits of entropy should be sufficient, we could go higher, but increasing
+  // this starts increasing the likelihood of collisions in filesystem hash
+  // structures. We could possibly go lower as well, but changing this would
+  // require a version bump, so 10 bits seems like a reasonable balance of
+  // number of fs entries vs path misses.
+  // If we find we need to increase the entropy, other implementations of
+  // directory fanning don't appear to use more than 16 bits of entropy in a
+  // single directory before starting to introduce additional fanning layers.
+  static const unsigned EntropyForIntermediateDir = 10;
+  static_assert(EntropyForIntermediateDir < 32,
+                "The logic below requires `EntropyForIntermediateDir` to be "
+                "less than 32 bits.");
+
+  // We're using hex, so we get 4 bits per character
+  static const unsigned CharactersForIntermediateDirName =
+    (EntropyForIntermediateDir + 3) / 4;
+  static const unsigned HashByteCount =
+    ((EntropyForIntermediateDir + 7) & ~7) / 8;
   static_assert(HashByteCount <= sizeof(uint32_t));
 
-  // We will never need 32 bits of entropy, so we just assert that now as it
-  // means we don't need to worry about overflows in the shifts below.
-  static_assert(TotalEntropyBits < 32);
-
   // It would be nice if there was a way to statically assert the size of an
-  // ArrayRef.
+  // ArrayRef. We are always going to copy 32 bits out, and it should always be
+  // larger than that.
   assert(I.Hash.size() >= sizeof(uint32_t));
 
   uint32_t Hash;
   memcpy(&Hash, I.Hash.data(), sizeof(Hash));
 
-  for (unsigned IntermediateDir = 0; IntermediateDir < IntermediateDirCount; ++IntermediateDir) {
-    unsigned DirValue = Hash & ((1u << EntropyPerIntermediateDir) - 1);
-    Hash >>= EntropyPerIntermediateDir;
-    char DirNameCharacters[CharactersPerDirName];
-    for (unsigned CharIdx = 0; CharIdx < CharactersPerDirName; ++CharIdx) {
-      // Mod and divide for clarity, this gets lowered to bit operations as
-      // expected.
-      unsigned CharValue = DirValue % 16;
-      DirValue /= 16;
-      DirNameCharacters[CharIdx] =
-        hexdigit(CharValue, /*LowerCase=*/false);
-    }
-    sys::path::append(PersistentPath, StringRef(DirNameCharacters, CharactersPerDirName));
+  // Mask off the unnecessary bits. The result is that the last character in the
+  // name may have less variance, but as we care about the total entropy of the
+  // name, this is fine.
+  unsigned DirValue = Hash & ((1u << EntropyForIntermediateDir) - 1);
+
+  char DirNameCharacters[CharactersForIntermediateDirName];
+  for (unsigned CharIdx = 0; CharIdx < CharactersForIntermediateDirName; ++CharIdx) {
+    // Mod and divide for clarity, this gets lowered to bitwise operations as
+    // expected.
+    unsigned CharValue = DirValue % 16;
+    DirValue /= 16;
+    DirNameCharacters[CharIdx] =
+      hexdigit(CharValue, /*LowerCase=*/false);
   }
+
+  StringRef IntermediateDirName =
+    StringRef(DirNameCharacters, CharactersForIntermediateDirName);
+  sys::path::append(PersistentPath, IntermediateDirName);
+
+  // Use a lambda here so that we can safely use a Twine repeatedly rather than
+  // performing an allocation, or duplicating the file name construction.
   auto AppendFileName = [&](Twine FileName) {
     sys::path::append(PersistentPath, FileName);
-
     if (TempPath)
       sys::path::append(*TempPath, FileName);
   };