[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();

69–75

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?

106–108

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

189–191

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.

69–75

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?

106–108

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

189–191

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.

261

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

106–108

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

106–108

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

106–108

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

106–108

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

89

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).

106–108

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

60

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...)

167

(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

 /// - a placeholder with an invalid stat indicating a not yet initialized entry.
 class CachedFileSystemEntry {
 public:
   /// Default constructor creates an entry with an invalid stat.
   CachedFileSystemEntry() : MaybeStat(llvm::vfs::Status()) {}
 
   CachedFileSystemEntry(std::error_code Error) : MaybeStat(std::move(Error)) {}
 
-  /// Create an entry that represents an opened source file with minimized or
-  /// original contents.
+  /// Create an entry that represents an opened source file with 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.
   static CachedFileSystemEntry createFileEntry(StringRef Filename,
-                                               llvm::vfs::FileSystem &FS,
-                                               bool Minimize = true);
+                                               llvm::vfs::FileSystem &FS);
+
+  /// Minimize contents of the file.
+  static void minimize(CachedFileSystemEntry &Entry);
 
   /// Create an entry that represents a directory on the filesystem.
   static CachedFileSystemEntry createDirectoryEntry(llvm::vfs::Status &&Stat);
 
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
   /// \returns True if the current entry points to a directory.
   bool isDirectory() const { return MaybeStat && MaybeStat->isDirectory(); }
 
+  /// \returns True if the file contents have already been read.
+  bool hasOriginalContents() const { return OriginalContents != nullptr; }
+
+  /// \returns True if the file contents have already been minimized.
+  bool hasMinimizedContents() const { return MinimizedContents != nullptr; }
+
   /// \returns The error or the file's contents.
-  llvm::ErrorOr<StringRef> getContents() const {
+  llvm::ErrorOr<StringRef> getContents(bool Minimized) const {
+    assert(isValid() && "not initialized");
     if (!MaybeStat)
       return MaybeStat.getError();
     assert(!MaybeStat->isDirectory() && "not a file");
-    assert(isValid() && "not initialized");
-    return Contents->getBuffer();
+    return (Minimized ? MinimizedContents : OriginalContents)->getBuffer();
   }
 
   /// \returns The error or the status of the entry.
-  llvm::ErrorOr<llvm::vfs::Status> getStatus() const {
+  llvm::ErrorOr<llvm::vfs::Status> getStatus(bool Minimized) const {
     assert(isValid() && "not initialized");
+    if (!MaybeStat || MaybeStat->isDirectory())
       return MaybeStat;
+    return llvm::vfs::Status::copyWithNewSize(*MaybeStat,
+                                              getContents(Minimized)->size());
   }
 
   /// \returns the name of the file.
   StringRef getName() const {
     assert(isValid() && "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;
+  std::unique_ptr<llvm::MemoryBuffer> MinimizedContents;
   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;
-
-  SingleCache &selectCache(bool Minimized) {
-    return Minimized ? CacheMinimized : CacheOriginal;
-  }
+  llvm::StringMap<const CachedFileSystemEntry *, llvm::BumpPtrAllocator> Cache;
 
 public:
-  void setCachedEntry(StringRef Filename, bool Minimized,
-                      const CachedFileSystemEntry *Entry) {
-    SingleCache &Cache = selectCache(Minimized);
+  void setCachedEntry(StringRef Filename, const CachedFileSystemEntry *Entry) {
     bool IsInserted = Cache.try_emplace(Filename, Entry).second;
     (void)IsInserted;
     assert(IsInserted && "local cache is updated more than once");
   }
 
-  const CachedFileSystemEntry *getCachedEntry(StringRef Filename,
-                                              bool Minimized) {
-    SingleCache &Cache = selectCache(Minimized);
+  const CachedFileSystemEntry *getCachedEntry(StringRef Filename) {
     auto It = Cache.find(Filename);
     return It == Cache.end() ? nullptr : It->getValue();
   }
 };
 
 /// A virtual file system optimized for the dependency discovery.
 ///
 /// It is primarily designed to work with source files whose contents was was
public:
   /// 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);
+  getOrCreateFileSystemEntry(const StringRef Filename, bool ShouldBeMinimized);
 
   /// Create a cached file system entry based on the initial status result.
   CachedFileSystemEntry
   createFileSystemEntry(llvm::ErrorOr<llvm::vfs::Status> &&MaybeStatus,
-                        StringRef Filename, bool ShouldMinimize);
+                        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;
 
 CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
-    StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize) {
+    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::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
       F.getBuffer(Stat->getName());
   if (!MaybeBuffer)
     return MaybeBuffer.getError();
 
-  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.
-    CachedFileSystemEntry Result;
-    Result.MaybeStat = std::move(*Stat);
-    Result.Contents = std::move(*MaybeBuffer);
-    return Result;
-  }
+  Result.OriginalContents = std::move(*MaybeBuffer);
 
-  CachedFileSystemEntry Result;
-  size_t Size = MinimizedFileContents.size();
-  Result.MaybeStat = llvm::vfs::Status(Stat->getName(), Stat->getUniqueID(),
-                                       Stat->getLastModificationTime(),
-                                       Stat->getUser(), Stat->getGroup(), Size,
-                                       Stat->getType(), Stat->getPermissions());
+void CachedFileSystemEntry::minimize(CachedFileSystemEntry &Result) {
+  llvm::SmallString<1024> MinimizedFileContents;
+  // Minimize the file down to directives that might affect the dependencies.
+  SmallVector<minimize_source_to_dependency_directives::Token, 64> Tokens;
+  if (minimizeSourceToDependencyDirectives(
+      Result.OriginalContents->getBuffer(), MinimizedFileContents, Tokens)) {
+    // FIXME: Propagate the diagnostic if desired by the client.
+    // Use the original file if the minimization failed.
+    Result.MinimizedContents =
+        llvm::MemoryBuffer::getMemBuffer(*Result.OriginalContents);
+    return;
+  }
+
   // The contents produced by the minimizer must be null terminated.
   assert(MinimizedFileContents.data()[MinimizedFileContents.size()] == '\0' &&
          "not null terminated contents");
-  Result.Contents = std::make_unique<llvm::SmallVectorMemoryBuffer>(
+  Result.MinimizedContents = 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;
   }
   Result.PPSkippedRangeMapping = std::move(Mapping);
-
-  return Result;
 }
 
 CachedFileSystemEntry
 CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status &&Stat) {
   assert(Stat.isDirectory() && "not a directory!");
   auto Result = CachedFileSystemEntry();
   Result.MaybeStat = std::move(Stat);
   return Result;
 }
 
-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::unique_lock<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);
 }
 
 CachedFileSystemEntry DependencyScanningWorkerFilesystem::createFileSystemEntry(
-    llvm::ErrorOr<llvm::vfs::Status> &&MaybeStatus, StringRef Filename,
-    bool ShouldMinimize) {
+    llvm::ErrorOr<llvm::vfs::Status> &&MaybeStatus, StringRef Filename) {
   if (!MaybeStatus)
     return CachedFileSystemEntry(MaybeStatus.getError());
 
   if (MaybeStatus->isDirectory())
     return CachedFileSystemEntry::createDirectoryEntry(std::move(*MaybeStatus));
 
-  return CachedFileSystemEntry::createFileEntry(Filename, getUnderlyingFS(),
-                                                ShouldMinimize);
+  return CachedFileSystemEntry::createFileEntry(Filename, getUnderlyingFS());
 }
 
 llvm::ErrorOr<const CachedFileSystemEntry *>
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-    const StringRef Filename) {
-  bool ShouldMinimize = shouldMinimize(Filename);
-
-  if (const auto *Entry = Cache.getCachedEntry(Filename, ShouldMinimize))
+    const StringRef Filename, bool ShouldBeMinimized) {
+  if (const auto *Entry = Cache.getCachedEntry(Filename))
     return Entry;
 
   // FIXME: Handle PCM/PCH files.
   // FIXME: Handle module map files.
 
   DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
-      &SharedCacheEntry = SharedCache.get(Filename, ShouldMinimize);
+      &SharedCacheEntry = SharedCache.get(Filename);
   const CachedFileSystemEntry *Result;
   {
     std::unique_lock<std::mutex> LockGuard(SharedCacheEntry.ValueLock);
     CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
     if (!CacheEntry.isValid()) {
       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 = createFileSystemEntry(std::move(MaybeStatus), Filename,
-                                         ShouldMinimize);
+      CacheEntry = createFileSystemEntry(std::move(MaybeStatus), Filename);
     }
 
+    if (ShouldBeMinimized && CacheEntry.hasOriginalContents() &&
+        !CacheEntry.hasMinimizedContents())
+      CachedFileSystemEntry::minimize(CacheEntry);
+
     Result = &CacheEntry;
   }
 
   // Store the result in the local cache.
-  Cache.setCachedEntry(Filename, ShouldMinimize, Result);
+  Cache.setCachedEntry(Filename, Result);
   return Result;
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
 DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
+  bool ShouldBeMinimized = shouldMinimize(Filename);
   const llvm::ErrorOr<const CachedFileSystemEntry *> Result =
-      getOrCreateFileSystemEntry(Filename);
+      getOrCreateFileSystemEntry(Filename, ShouldBeMinimized);
   if (!Result)
     return Result.getError();
-  return (*Result)->getStatus();
+  return (*Result)->getStatus(ShouldBeMinimized);
 }
 
 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(const CachedFileSystemEntry *Entry, bool ShouldBeMinimized,
          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,
+    const CachedFileSystemEntry *Entry, bool ShouldBeMinimized,
     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(ShouldBeMinimized);
   if (!Contents)
     return Contents.getError();
   auto Result = std::make_unique<MinimizedVFSFile>(
       llvm::MemoryBuffer::getMemBuffer(*Contents, Entry->getName(),
                                        /*RequiresNullTerminator=*/false),
-      *Entry->getStatus());
+      *Entry->getStatus(ShouldBeMinimized));
   if (!Entry->getPPSkippedRangeMapping().empty() && PPSkipMappings)
     (*PPSkipMappings)[Result->Buffer->getBufferStart()] =
         &Entry->getPPSkippedRangeMapping();
   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);
 
+  bool ShouldBeMinimized = shouldMinimize(Filename);
   const llvm::ErrorOr<const CachedFileSystemEntry *> Result =
-      getOrCreateFileSystemEntry(Filename);
+      getOrCreateFileSystemEntry(Filename, ShouldBeMinimized);
   if (!Result)
     return Result.getError();
-  return MinimizedVFSFile::create(Result.get(), PPSkipMappings);
+  return MinimizedVFSFile::create(Result.get(), ShouldBeMinimized,
+                                  PPSkipMappings);
 }

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(),