[clang][deps] Squash caches for original and minimized files

[jansvoboda11]

Summary

The minimizing and caching filesystem used by the dependency scanner keeps minimized and original files in separate caches.

This setup is not well suited for dealing with files that are sometimes minimized and sometimes not. Such files are being stat-ed and read twice, which is wasteful and also means the two versions of the file can get "out of sync".

This patch squashes the two caches together. When a file is stat-ed or read, its original contents are populated. If a file needs to be minimized, we give the minimizer the already loaded contents instead of reading the file again.

[jansvoboda11]

Update documentation

[dexonsmith]

This looks really nice.

One problem is that it's subtle to confirm whether it's thread-safe. The local cache access parts of CachedFileSystemEntry lock-free, but the CachedFileSystemEntry might get changed later (i.e., filled in with minimized content). Are accessed fields guaranteed by context not to be the ones that mutate later?

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

50–51

Might be more natural as a non-static:

void minimize();

63–95

I *think* this is thread-safe -- since Minimized should be the same as when the local cache entry was created -- but it's a bit subtle.

The problematic case I am worried about:

• First use in local cache is non-minimized.
  • Creates shared cache entry that's not minimized.
• Some other local cache wants it to be minimized.
• Later use in local cache is minimized.
  • Accessing Minimized pointer races with the other thread setting it.

If the local cache is guaranteed to always pass the same value for Minimized as when it fist accessed it, then there shouldn't be a problem...

I wonder if there's a way to make it less subtle?

117–125

I'm finding it a bit subtle detecting if there are races on access / setting of these, but I think it's correct.

• I think I verified that they are "set once".
• All the setters are guarded by locks.
• The first getter per "local" cache is guarded by a lock.
• Subsequent getters are not.

The key question: are the subsequent getters ONLY used when the first getter was successful?

One way to make it more obvious:

  struct ContentWithPPRanges {
    std::unique_ptr<llvm::MemoryBuffer> Content;
    PreprocessorSkippedRangeMapping PPSkippedRangeMapping;
  };

private:
  // Always accessed,mutated under a lock. Not mutated after they escape.
  std::unique_ptr<llvm::MemoryBuffer> Original;
  std::unique_ptr<llvm::MemoryBuffer> Minimized;
  PreprocessorSkippedRangeMapping PPSkippedRangeMapping;

  // Used in getters. Pointed-at memory immutable after these are set.
  std::atomic<const llvm::MemoryBuffer *> OriginalAccess;
  std::atomic<const llvm::MemoryBuffer *> MinimizedAccess;
  std::atomic<const PreprocessorSkippedRangeMapping *> PPRangesAccess;

I don't think this is the only approach though.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

188–190

Is the DependencyScanningWorkerFilesystem guaranteed to always want either minimized or not minimized?

IOW, if the same filesystem is reused, and the first time only the original file was needed and later the minimized was needed, I don't see the code path for minimizing the file later. But maybe reusing one of these FSs is not supported?

[jansvoboda11]

Introduce EntryRef, handle delayed minimization.

[jansvoboda11]

Prevent potential data races by wrapping access to Entry.getPPSkippedRangeMapping() in if (Entry.isMinimized()).

[jansvoboda11]

In D115346#3181198, @dexonsmith wrote:

This looks really nice.

One problem is that it's subtle to confirm whether it's thread-safe. The local cache access parts of CachedFileSystemEntry lock-free, but the CachedFileSystemEntry might get changed later (i.e., filled in with minimized content). Are accessed fields guaranteed by context not to be the ones that mutate later?

I believe the latest revision is thread-safe (albeit with benign data-races on needsMinimization). Once a CachedFileSystemEntry has been created, only the MinimizedContents and PPSkippedRangeMapping members can be modified (at most once, under a lock). Access to those fields only happens in "minimizing" contexts.

[jansvoboda11]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

50–51

Agreed. I originally wanted to minimize diff of the implementation (by being able to refer to Result as before), but I'm happy making this an instance method.

63–95

I extracted the logic into EntryRef which now gets returned from the main function. It carries the bool Minimized bit and makes reasoning about this more local. WDYT?

117–125

I think there are no races on the original contents. The pointer is unconditionally set on creation of CachedFileSystemEntry under a lock that no threads get past without having set the pointer (or failing and never accessing the pointer).

For minimized contents, the latest revision adds check at the beginning of the main function (needsMinimization) outside the critical section. There are three paths I can think of:

• The check returns true in thread A (pointer is null), thread A enters critical section, minimizes the contents and initializes the pointer.
• The check returns true in thread A, but thread B entered the critical section, minimized contents and initialized the pointer. When thread A enters the critical section, it performs the check again, figures that out and skips minimization.
• The check returns false and the local cache entry is returned.

So there definitely is a race here, but I believe it's benign. Do you agree? Do you think it's worth addressing?

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

188–190

You're right, minimization of a particular file can be turned on/off on the fly. I already have a test for this in D114966 but somehow dropped it when extracting this patch. Fixed in the latest revision.

255

Maybe the isMinimized() check should be performed internally in EntryRef when accessing getPPSkippedRangeMapping() to make the race avoidance more local...

[dexonsmith]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

117–125

I don't trust myself to evaluate whether it's benign, but if there's no atomic mutation, then I think it's possible that when the setter changes a value from "x" to "y" then the racing reader can see a third value "z". You might gain some confidence by using -fsanitize=thread on a workload that's going to include this sort of thing -- probably hard to exercise: PCH already exists, try minimizing something that uses the PCH, and then try minimizing something that doesn't.

I'd rather just avoid the race entirely so we don't need to guess though.

[jansvoboda11]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

117–125

Interesting...

After reading up on this a bit, my understanding is that reads of MinimizedContents cannot be torn, because it's pointers-sized and aligned. So we should never see a third value "z". Am I wrong?

The potential data race is IMO somewhat independent from the read tearing aspect and is avoided by defensively checking MinimizedContents again under lock.

To confirm, I ran the following test case with and without thread sanitizer, never seeing data races or incorrect results.

clang-scan-deps-race-test.patch2 KBDownload

I'm happy to use the std::atomic pattern you suggested, but I want to be sure I understand why that's necessary.

[dexonsmith]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

117–125

Heh, I don't know what can and cannot tear (especially on different architectures/etc.), I'm just wary. I'll trust your homework, but please add a comment documenting why it's thread-safe to read without atomics/locks.

[jansvoboda11]

Rebase, use std::atomic to prevent data races.

[jansvoboda11]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

117–125

I believe I got to the bottom of it. The pattern I'm using is called "double-checked locking" and is considered unsafe without atomics, since compiler optimizations can break it on some platforms. Adding a memory fence (std::atomic) here should make this safe and portable.

[dexonsmith]

LGTM after you fix one more race (and add good code comments). Reads of PPSkippedRangeMappings don't happen until after MinimizedContentsAccess is checked, but the writes need to happen in reverse order to properly guard.

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

94

I think the .load() is unnecessary due to atomic<T>::operator T() const, but up to you; arguably it helps readability here (as redundant "documentation" that it's an atomic load).

117–125

Okay, makes sense.

(IMO, storing it twice is silly. We should just have an atomic variant of unique_ptr, maybe called ThreadSafeUniquePtr, which uses std::atomic<T*> and has no customizable deleter (or requires deleter to have no storage; or adds a mutex if/when deleter needs storage). But not for this patch...)

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

59

I think you should move this line *below* the assignment to PPSkippedRangeMapping so that no one tries to read that until it's ready. (I have a few other comments inline to point out why; please add a good comment explaining it in code too...)

Also, I think you can just write this as:

MinimizedContentsAccess = MinimizedContentsStorage.get();

76–85

(PPSkippedRangeMapping isn't ready to be read until here...)

166

(I think needsUpdate() can return false as soon as MinimizedContentsAccess is set, but PPSkippedRangeMappings won't be ready to read yet.)

[jansvoboda11]

Committed with the writes swapped and explanation in the comment. Thanks for the review!

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

 namespace clang {
 namespace tooling {
 namespace dependencies {
 
 /// An in-memory representation of a file system entity that is of interest to
 /// the dependency scanning filesystem.
 ///
 /// It represents one of the following:
-/// - an opened source file with minimized contents and a stat value.
-/// - an opened source file with original contents and a stat value.
-/// - a directory entry with its stat value.
-/// - an error value to represent a file system error.
+/// - opened file with original contents and a stat value,
+/// - opened file with original contents, minimized contents and a stat value,
+/// - directory entry with its stat value,
+/// - filesystem error,
 /// - uninitialized entry with unknown status.
 class CachedFileSystemEntry {
 public:
   /// Creates an uninitialized entry.
-  CachedFileSystemEntry() : MaybeStat(llvm::vfs::Status()) {}
+  CachedFileSystemEntry()
+      : MaybeStat(llvm::vfs::Status()), MinimizedContentsAccess(nullptr) {}
 
   /// Initialize the cached file system entry.
   void init(llvm::ErrorOr<llvm::vfs::Status> &&MaybeStatus, StringRef Filename,
-            llvm::vfs::FileSystem &FS, bool ShouldMinimize = true);
+            llvm::vfs::FileSystem &FS);
 
   /// Initialize the entry as file with minimized or original contents.
   ///
   /// The filesystem opens the file even for `stat` calls open to avoid the
   /// issues with stat + open of minimized files that might lead to a
   /// mismatching size of the file.
-  llvm::ErrorOr<llvm::vfs::Status>
-  initFile(StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize = true);
+  llvm::ErrorOr<llvm::vfs::Status> initFile(StringRef Filename,
+                                            llvm::vfs::FileSystem &FS);
+
+  /// Minimize contents of the file.
+  void minimizeFile();
 
   /// \returns True if the entry is initialized.
   bool isInitialized() const {
     return !MaybeStat || MaybeStat->isStatusKnown();
   }
 
   /// \returns True if the current entry points to a directory.
   bool isDirectory() const { return MaybeStat && MaybeStat->isDirectory(); }
 
-  /// \returns The error or the file's contents.
-  llvm::ErrorOr<StringRef> getContents() const {
+  /// \returns The error or the file's original contents.
+  llvm::ErrorOr<StringRef> getOriginalContents() const {
+    if (!MaybeStat)
+      return MaybeStat.getError();
+    assert(!MaybeStat->isDirectory() && "not a file");
+    assert(isInitialized() && "not initialized");
+    assert(OriginalContents && "not read");
+    return OriginalContents->getBuffer();
+  }
+
+  /// \returns The error or the file's minimized contents.
+  llvm::ErrorOr<StringRef> getMinimizedContents() const {
     if (!MaybeStat)
       return MaybeStat.getError();
     assert(!MaybeStat->isDirectory() && "not a file");
     assert(isInitialized() && "not initialized");
-    return Contents->getBuffer();
+    llvm::MemoryBuffer *Buffer = MinimizedContentsAccess.load();
+    assert(Buffer && "not minimized");
+    return Buffer->getBuffer();
+  }
+
+  /// \returns True if this entry represents a file that can be read.
+  bool isReadable() const { return MaybeStat && !MaybeStat->isDirectory(); }
+
+  /// \returns True if this cached entry needs to be updated.
+  bool needsUpdate(bool ShouldBeMinimized) const {
+    return isReadable() && needsMinimization(ShouldBeMinimized);
+  }
+
+  /// \returns True if the contents of this entry need to be minimized.
+  bool needsMinimization(bool ShouldBeMinimized) const {
+    return ShouldBeMinimized && !MinimizedContentsAccess.load();
   }
 
   /// \returns The error or the status of the entry.
   llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
     assert(isInitialized() && "not initialized");
     return MaybeStat;
   }
 
   /// \returns the name of the file.
   StringRef getName() const {
     assert(isInitialized() && "not initialized");
     return MaybeStat->getName();
   }
 
   /// Return the mapping between location -> distance that is used to speed up
   /// the block skipping in the preprocessor.
   const PreprocessorSkippedRangeMapping &getPPSkippedRangeMapping() const {
     return PPSkippedRangeMapping;
   }
 
-  CachedFileSystemEntry(CachedFileSystemEntry &&) = default;
-  CachedFileSystemEntry &operator=(CachedFileSystemEntry &&) = default;
-
-  CachedFileSystemEntry(const CachedFileSystemEntry &) = delete;
-  CachedFileSystemEntry &operator=(const CachedFileSystemEntry &) = delete;
-
 private:
   llvm::ErrorOr<llvm::vfs::Status> MaybeStat;
-  std::unique_ptr<llvm::MemoryBuffer> Contents;
+  std::unique_ptr<llvm::MemoryBuffer> OriginalContents;
+
+  /// Owning storage for the minimized file contents.
+  std::unique_ptr<llvm::MemoryBuffer> MinimizedContentsStorage;
+  /// Atomic view of the minimized file contents.
+  /// This prevents data races when multiple threads call `needsMinimization`.
+  std::atomic<llvm::MemoryBuffer *> MinimizedContentsAccess;
+
   PreprocessorSkippedRangeMapping PPSkippedRangeMapping;
 };
 
 /// This class is a shared cache, that caches the 'stat' and 'open' calls to the
 /// underlying real file system. It distinguishes between minimized and original
 /// files.
 ///
 /// It is sharded based on the hash of the key to reduce the lock contention for
 /// the worker threads.
 class DependencyScanningFilesystemSharedCache {
 public:
   struct SharedFileSystemEntry {
     std::mutex ValueLock;
     CachedFileSystemEntry Value;
   };
 
+  DependencyScanningFilesystemSharedCache();
+
   /// Returns a cache entry for the corresponding key.
   ///
   /// A new cache entry is created if the key is not in the cache. This is a
   /// thread safe call.
-  SharedFileSystemEntry &get(StringRef Key, bool Minimized);
-
-private:
-  class SingleCache {
-  public:
-    SingleCache();
-
-    SharedFileSystemEntry &get(StringRef Key);
 
-  private:
-    struct CacheShard {
-      std::mutex CacheLock;
-      llvm::StringMap<SharedFileSystemEntry, llvm::BumpPtrAllocator> Cache;
-    };
-    std::unique_ptr<CacheShard[]> CacheShards;
-    unsigned NumShards;
-  };
 
-  SingleCache CacheMinimized;
-  SingleCache CacheOriginal;
-};
-
 /// This class is a local cache, that caches the 'stat' and 'open' calls to the
 /// underlying real file system. It distinguishes between minimized and original
 /// files.
 class DependencyScanningFilesystemLocalCache {
-private:
-  using SingleCache =
-      llvm::StringMap<const CachedFileSystemEntry *, llvm::BumpPtrAllocator>;
-
-  SingleCache CacheMinimized;
-  SingleCache CacheOriginal;
+  llvm::StringMap<const CachedFileSystemEntry *, llvm::BumpPtrAllocator> Cache;
 
-  SingleCache &selectCache(bool Minimized) {
-    return Minimized ? CacheMinimized : CacheOriginal;
+public:
+  const CachedFileSystemEntry *getCachedEntry(StringRef Filename) {
+    return Cache[Filename];
   }
+};
+
+/// Reference to a CachedFileSystemEntry.
+/// If the underlying entry is an opened file, this wrapper returns the correct
+/// contents (original or minimized) and ensures consistency with file size
+/// reported by status.
+class EntryRef {
+  /// For entry that is an opened file, this bit signifies whether its contents
+  /// are minimized.
+  bool Minimized;
+
+  /// The underlying cached entry.
+  const CachedFileSystemEntry *Entry;
 
 public:
-  void setCachedEntry(StringRef Filename, bool Minimized,
-                      const CachedFileSystemEntry *Entry) {
-    SingleCache &Cache = selectCache(Minimized);
-    bool IsInserted = Cache.try_emplace(Filename, Entry).second;
-    (void)IsInserted;
-    assert(IsInserted && "local cache is updated more than once");
+  EntryRef(bool Minimized, const CachedFileSystemEntry *Entry)
+      : Minimized(Minimized), Entry(Entry) {}
+
+  llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
+    auto MaybeStat = Entry->getStatus();
+    if (!MaybeStat || MaybeStat->isDirectory())
+      return MaybeStat;
+    return llvm::vfs::Status::copyWithNewSize(*MaybeStat,
+                                              getContents()->size());
+  }
+
+  bool isDirectory() const { return Entry->isDirectory(); }
+
+  StringRef getName() const { return Entry->getName(); }
+
+  llvm::ErrorOr<StringRef> getContents() const {
+    return Minimized ? Entry->getMinimizedContents()
+                     : Entry->getOriginalContents();
   }
 
-  const CachedFileSystemEntry *getCachedEntry(StringRef Filename,
-                                              bool Minimized) {
-    SingleCache &Cache = selectCache(Minimized);
-    auto It = Cache.find(Filename);
-    return It == Cache.end() ? nullptr : It->getValue();
+  const PreprocessorSkippedRangeMapping *getPPSkippedRangeMapping() const {
+    return Minimized ? &Entry->getPPSkippedRangeMapping() : nullptr;
   }
 };
 
 /// A virtual file system optimized for the dependency discovery.
 ///
 /// It is primarily designed to work with source files whose contents was was
 /// preprocessed to remove any tokens that are unlikely to affect the dependency
 /// computation.
public:
   void disableMinimization(StringRef Filename);
   /// Enable minimization of all files.
   void enableMinimizationOfAllFiles() { NotToBeMinimized.clear(); }
 
 private:
   /// Check whether the file should be minimized.
   bool shouldMinimize(StringRef Filename);
 
-  llvm::ErrorOr<const CachedFileSystemEntry *>
-  getOrCreateFileSystemEntry(const StringRef Filename);
+  llvm::ErrorOr<EntryRef> getOrCreateFileSystemEntry(StringRef Filename);
 
   /// The global cache shared between worker threads.
   DependencyScanningFilesystemSharedCache &SharedCache;
   /// The local cache is used by the worker thread to cache file system queries
   /// locally instead of querying the global cache every time.
   DependencyScanningFilesystemLocalCache Cache;
   /// The optional mapping structure which records information about the
   /// excluded conditional directive skip mappings that are used by the

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 #include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
 llvm::ErrorOr<llvm::vfs::Status>
-CachedFileSystemEntry::initFile(StringRef Filename, llvm::vfs::FileSystem &FS,
-                                bool Minimize) {
+CachedFileSystemEntry::initFile(StringRef Filename, llvm::vfs::FileSystem &FS) {
   // Load the file and its content from the file system.
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MaybeFile =
       FS.openFileForRead(Filename);
   if (!MaybeFile)
     return MaybeFile.getError();
 
   llvm::ErrorOr<llvm::vfs::Status> Stat = (*MaybeFile)->status();
   if (!Stat)
     return Stat.getError();
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
       (*MaybeFile)->getBuffer(Stat->getName());
   if (!MaybeBuffer)
     return MaybeBuffer.getError();
 
+  OriginalContents = std::move(*MaybeBuffer);
+  return Stat;
+}
+
+void CachedFileSystemEntry::minimizeFile() {
+  assert(OriginalContents && "minimizing missing contents");
+
   llvm::SmallString<1024> MinimizedFileContents;
   // Minimize the file down to directives that might affect the dependencies.
-  const auto &Buffer = *MaybeBuffer;
   SmallVector<minimize_source_to_dependency_directives::Token, 64> Tokens;
-  if (!Minimize || minimizeSourceToDependencyDirectives(
-                       Buffer->getBuffer(), MinimizedFileContents, Tokens)) {
-    // Use the original file unless requested otherwise, or
-    // if the minimization failed.
-    // FIXME: Propage the diagnostic if desired by the client.
-    Contents = std::move(*MaybeBuffer);
-    return Stat;
+  if (minimizeSourceToDependencyDirectives(OriginalContents->getBuffer(),
+                                           MinimizedFileContents, Tokens)) {
+    // FIXME: Propagate the diagnostic if desired by the client.
+    // Use the original file if the minimization failed.
+    MinimizedContentsStorage =
+        llvm::MemoryBuffer::getMemBuffer(*OriginalContents);
+    MinimizedContentsAccess.store(MinimizedContentsStorage.get());
+    return;
   }
 
-  Stat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-                           Stat->getLastModificationTime(), Stat->getUser(),
-                           Stat->getGroup(), MinimizedFileContents.size(),
-                           Stat->getType(), Stat->getPermissions());
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
          "not null terminated contents");
-  Contents = std::make_unique<llvm::SmallVectorMemoryBuffer>(
-      std::move(MinimizedFileContents));
 
   // Compute the skipped PP ranges that speedup skipping over inactive
   // preprocessor blocks.
   llvm::SmallVector<minimize_source_to_dependency_directives::SkippedRange, 32>
       SkippedRanges;
   minimize_source_to_dependency_directives::computeSkippedRanges(Tokens,
                                                                  SkippedRanges);
   PreprocessorSkippedRangeMapping Mapping;
   for (const auto &Range : SkippedRanges) {
     if (Range.Length < 16) {
       // Ignore small ranges as non-profitable.
       // FIXME: This is a heuristic, its worth investigating the tradeoffs
       // when it should be applied.
       continue;
     }
     Mapping[Range.Offset] = Range.Length;
   }
   PPSkippedRangeMapping = std::move(Mapping);
-  return Stat;
+
+  MinimizedContentsStorage = std::make_unique<llvm::SmallVectorMemoryBuffer>(
+      std::move(MinimizedFileContents));
+  // The algorithm in `getOrCreateFileSystemEntry` uses the presence of
+  // minimized contents to decide whether an entry is up-to-date or not.
+  // If it is up-to-date, the skipped range mappings must be already computed.
+  // This is why we need to store the minimized contents **after** storing the
+  // skipped range mappings. Failing to do so would lead to a data race.
+  MinimizedContentsAccess.store(MinimizedContentsStorage.get());
 }
 
-DependencyScanningFilesystemSharedCache::SingleCache::SingleCache() {
+DependencyScanningFilesystemSharedCache::
+    DependencyScanningFilesystemSharedCache() {
   // This heuristic was chosen using a empirical testing on a
   // reasonably high core machine (iMacPro 18 cores / 36 threads). The cache
   // sharding gives a performance edge by reducing the lock contention.
   // FIXME: A better heuristic might also consider the OS to account for
   // the different cost of lock contention on different OSes.
   NumShards =
       std::max(2u, llvm::hardware_concurrency().compute_thread_count() / 4);
   CacheShards = std::make_unique<CacheShard[]>(NumShards);
 }
 
 DependencyScanningFilesystemSharedCache::SharedFileSystemEntry &
-DependencyScanningFilesystemSharedCache::SingleCache::get(StringRef Key) {
+DependencyScanningFilesystemSharedCache::get(StringRef Key) {
   CacheShard &Shard = CacheShards[llvm::hash_value(Key) % NumShards];
   std::lock_guard<std::mutex> LockGuard(Shard.CacheLock);
   auto It = Shard.Cache.try_emplace(Key);
   return It.first->getValue();
 }
 
-DependencyScanningFilesystemSharedCache::SharedFileSystemEntry &
-DependencyScanningFilesystemSharedCache::get(StringRef Key, bool Minimized) {
-  SingleCache &Cache = Minimized ? CacheMinimized : CacheOriginal;
-  return Cache.get(Key);
-}
-
 /// Whitelist file extensions that should be minimized, treating no extension as
 /// a source file that should be minimized.
 ///
 /// This is kinda hacky, it would be better if we knew what kind of file Clang
 /// was expecting instead.
 static bool shouldMinimizeBasedOnExtension(StringRef Filename) {
   StringRef Ext = llvm::sys::path::extension(Filename);
   if (Ext.empty())
if (!shouldMinimizeBasedOnExtension(RawFilename))
     return false;
 
   llvm::SmallString<256> Filename;
   llvm::sys::path::native(RawFilename, Filename);
   return !NotToBeMinimized.contains(Filename);
 }
 
 void CachedFileSystemEntry::init(llvm::ErrorOr<llvm::vfs::Status> &&MaybeStatus,
-                                 StringRef Filename, llvm::vfs::FileSystem &FS,
-                                 bool ShouldMinimize) {
+                                 StringRef Filename,
+                                 llvm::vfs::FileSystem &FS) {
   if (!MaybeStatus || MaybeStatus->isDirectory())
     MaybeStat = std::move(MaybeStatus);
   else
-    MaybeStat = initFile(Filename, FS, ShouldMinimize);
+    MaybeStat = initFile(Filename, FS);
 }
 
-llvm::ErrorOr<const CachedFileSystemEntry *>
+llvm::ErrorOr<EntryRef>
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-    const StringRef Filename) {
-  bool ShouldMinimize = shouldMinimize(Filename);
+    StringRef Filename) {
+  bool ShouldBeMinimized = shouldMinimize(Filename);
 
-  if (const auto *Entry = Cache.getCachedEntry(Filename, ShouldMinimize))
-    return Entry;
+  const auto *Entry = Cache.getCachedEntry(Filename);
+  if (Entry && !Entry->needsUpdate(ShouldBeMinimized))
+    return EntryRef(ShouldBeMinimized, Entry);
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
-  DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-      &SharedCacheEntry = SharedCache.get(Filename, ShouldMinimize);
-  const CachedFileSystemEntry *Result;
+  auto &SharedCacheEntry = SharedCache.get(Filename);
   {
     std::lock_guard<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
     if (!CacheEntry.isInitialized()) {
       auto MaybeStatus = getUnderlyingFS().status(Filename);
       if (!MaybeStatus && !shouldCacheStatFailures(Filename))
         // HACK: We need to always restat non source files if the stat fails.
         //   This is because Clang first looks up the module cache and module
         //   files before building them, and then looks for them again. If we
         //   cache the stat failure, it won't see them the second time.
         return MaybeStatus.getError();
-      CacheEntry.init(std::move(MaybeStatus), Filename, getUnderlyingFS(),
-                      ShouldMinimize);
+      CacheEntry.init(std::move(MaybeStatus), Filename, getUnderlyingFS());
     }
 
-    Result = &CacheEntry;
+    // Checking `needsUpdate` verifies the entry represents an opened file.
+    // Only checking `needsMinimization` could lead to minimization of files
+    // that we failed to load (such files don't have `OriginalContents`).
+    if (CacheEntry.needsUpdate(ShouldBeMinimized))
+      CacheEntry.minimizeFile();
   }
 
   // Store the result in the local cache.
-  Cache.setCachedEntry(Filename, ShouldMinimize, Result);
-  return Result;
+  Entry = &SharedCacheEntry.Value;
+  return EntryRef(ShouldBeMinimized, Entry);
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
 DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
-  const llvm::ErrorOr<const CachedFileSystemEntry *> Result =
-      getOrCreateFileSystemEntry(Filename);
+
+  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
   if (!Result)
     return Result.getError();
-  return (*Result)->getStatus();
+  return Result->getStatus();
 }
 
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
 /// this subclass.
 class MinimizedVFSFile final : public llvm::vfs::File {
 public:
   MinimizedVFSFile(std::unique_ptr<llvm::MemoryBuffer> Buffer,
                    llvm::vfs::Status Stat)
       : Buffer(std::move(Buffer)), Stat(std::move(Stat)) {}
 
   static llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
-  create(const CachedFileSystemEntry *Entry,
+  create(EntryRef Entry,
          ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings);
 
   llvm::ErrorOr<llvm::vfs::Status> status() override { return Stat; }
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
             bool IsVolatile) override {
     return std::move(Buffer);
   }
 
   std::error_code close() override { return {}; }
 
 private:
   std::unique_ptr<llvm::MemoryBuffer> Buffer;
   llvm::vfs::Status Stat;
 };
 
 } // end anonymous namespace
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MinimizedVFSFile::create(
-    const CachedFileSystemEntry *Entry,
-    ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
-  if (Entry->isDirectory())
+    EntryRef Entry, ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
+  if (Entry.isDirectory())
     return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
         std::make_error_code(std::errc::is_a_directory));
-  llvm::ErrorOr<StringRef> Contents = Entry->getContents();
+  llvm::ErrorOr<StringRef> Contents = Entry.getContents();
   if (!Contents)
     return Contents.getError();
   auto Result = std::make_unique<MinimizedVFSFile>(
-      llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
+      llvm::MemoryBuffer::getMemBuffer(*Contents, Entry.getName(),
                                        /*RequiresNullTerminator=*/false),
-      *Entry->getStatus());
-  if (!Entry->getPPSkippedRangeMapping().empty() && PPSkipMappings)
-    (*PPSkipMappings)[Result->Buffer->getBufferStart()] =
-        &Entry->getPPSkippedRangeMapping();
+      *Entry.getStatus());
+
+  const auto *EntrySkipMappings = Entry.getPPSkippedRangeMapping();
+  if (EntrySkipMappings && !EntrySkipMappings->empty() && PPSkipMappings)
+    (*PPSkipMappings)[Result->Buffer->getBufferStart()] = EntrySkipMappings;
+
   return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
       std::unique_ptr<llvm::vfs::File>(std::move(Result)));
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
-  const llvm::ErrorOr<const CachedFileSystemEntry *> Result =
-      getOrCreateFileSystemEntry(Filename);
+  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
   if (!Result)
     return Result.getError();
   return MinimizedVFSFile::create(Result.get(), PPSkipMappings);
 }

clang/unittests/Tooling/DependencyScannerTest.cpp

TEST(DependencyScanner, ScanDepsReuseFilemanagerHasInclude) {
   EXPECT_EQ(convert_to_slash(Deps[1]), "/root/header.h");
   EXPECT_EQ(convert_to_slash(Deps[2]), "/root/symlink.h");
   EXPECT_EQ(convert_to_slash(Deps[3]), "/root/test.cpp");
   EXPECT_EQ(convert_to_slash(Deps[4]), "/root/header.h");
   EXPECT_EQ(convert_to_slash(Deps[5]), "/root/symlink.h");
 }
 
 namespace dependencies {
-TEST(DependencyScanningFilesystem, IgnoredFilesHaveSeparateCache) {
+TEST(DependencyScanningFilesystem, IgnoredFilesAreCachedSeparately1) {
   auto VFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-  VFS->addFile("/mod.h", 0, llvm::MemoryBuffer::getMemBuffer("// hi there!\n"));
+  VFS->addFile("/mod.h", 0,
+               llvm::MemoryBuffer::getMemBuffer("#include <foo.h>\n"
+                                                "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
   auto Mappings = std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
   DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
 
+  DepFS.enableMinimizationOfAllFiles(); // Let's be explicit for clarity.
   auto StatusMinimized0 = DepFS.status("/mod.h");
   DepFS.disableMinimization("/mod.h");
   auto StatusFull1 = DepFS.status("/mod.h");
-  DepFS.enableMinimizationOfAllFiles();
-
-  auto StatusMinimized2 = DepFS.status("/mod.h");
-  DepFS.disableMinimization("/mod.h");
-  auto StatusFull3 = DepFS.status("/mod.h");
 
   EXPECT_TRUE(StatusMinimized0);
-  EXPECT_EQ(StatusMinimized0->getSize(), 0u);
   EXPECT_TRUE(StatusFull1);
-  EXPECT_EQ(StatusFull1->getSize(), 13u);
+  EXPECT_EQ(StatusMinimized0->getSize(), 17u);
+  EXPECT_EQ(StatusFull1->getSize(), 30u);
+}
+
+TEST(DependencyScanningFilesystem, IgnoredFilesAreCachedSeparately2) {
+  auto VFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  VFS->addFile("/mod.h", 0,
+               llvm::MemoryBuffer::getMemBuffer("#include <foo.h>\n"
+                                                "// hi there!\n"));
+
+  DependencyScanningFilesystemSharedCache SharedCache;
+  auto Mappings = std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+
+  DepFS.disableMinimization("/mod.h");
+  auto StatusFull0 = DepFS.status("/mod.h");
+  DepFS.enableMinimizationOfAllFiles();
+  auto StatusMinimized1 = DepFS.status("/mod.h");
 
-  EXPECT_TRUE(StatusMinimized2);
-  EXPECT_EQ(StatusMinimized2->getSize(), 0u);
-  EXPECT_TRUE(StatusFull3);
-  EXPECT_EQ(StatusFull3->getSize(), 13u);
+  EXPECT_TRUE(StatusFull0);
+  EXPECT_TRUE(StatusMinimized1);
+  EXPECT_EQ(StatusFull0->getSize(), 30u);
+  EXPECT_EQ(StatusMinimized1->getSize(), 17u);
 }
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang

llvm/include/llvm/Support/VirtualFileSystem.h

public:
 
   Status() = default;
   Status(const llvm::sys::fs::file_status &Status);
   Status(const Twine &Name, llvm::sys::fs::UniqueID UID,
          llvm::sys::TimePoint<> MTime, uint32_t User, uint32_t Group,
          uint64_t Size, llvm::sys::fs::file_type Type,
          llvm::sys::fs::perms Perms);
 
+  /// Get a copy of a Status with a different size.
+  static Status copyWithNewSize(const Status &In, uint64_t NewSize);
   /// Get a copy of a Status with a different name.
   static Status copyWithNewName(const Status &In, const Twine &NewName);
   static Status copyWithNewName(const llvm::sys::fs::file_status &In,
                                 const Twine &NewName);
 
   /// Returns the name that should be used for this file or directory.
   StringRef getName() const { return Name; }
 

llvm/lib/Support/VirtualFileSystem.cpp

: UID(Status.getUniqueID()), MTime(Status.getLastModificationTime()),
       Type(Status.type()), Perms(Status.permissions()) {}
 
 Status::Status(const Twine &Name, UniqueID UID, sys::TimePoint<> MTime,
                uint32_t User, uint32_t Group, uint64_t Size, file_type Type,
                perms Perms)
     : Name(Name.str()), UID(UID), MTime(MTime), User(User), Group(Group),
       Size(Size), Type(Type), Perms(Perms) {}
 
+Status Status::copyWithNewSize(const Status &In, uint64_t NewSize) {
+  return Status(In.getName(), In.getUniqueID(), In.getLastModificationTime(),
+                In.getUser(), In.getGroup(), NewSize, In.getType(),
+                In.getPermissions());
+}
+
 Status Status::copyWithNewName(const Status &In, const Twine &NewName) {
   return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
                 In.getUser(), In.getGroup(), In.getSize(), In.getType(),
                 In.getPermissions());
 }
 
 Status Status::copyWithNewName(const file_status &In, const Twine &NewName) {
   return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),