[clang][deps] Avoid reading file for stat calls

[jansvoboda11]

Summary

The minimizing FS defensively reads files even for stat calls. This is necessary to report consistent file sizes for stat calls and for read calls that may minimize the file, thus changing its size.

This patch avoids reading files that are not to be minimized. This is more efficient in scenarios a non-minimized file is only stat-ed during dependency scan and never open-ed.

Depends on D114966.

[jansvoboda11]

Rebase

[jansvoboda11]

Assuming the filesystem doesn't change during dependency scanning, this change keeps the consistency between stat/read calls for minimized files.

Thinking about it some more though, the original reason for reading files eagerly (even for stat calls) was most likely to avoid inconsistencies when the file changes on the filesystem between stat and read. Is that correct, @arphaman? If so, I think I should abandon this patch.

[dexonsmith]

In D114968#3167519, @jansvoboda11 wrote:

Assuming the filesystem doesn't change during dependency scanning, this change keeps the consistency between stat/read calls for minimized files.

Thinking about it some more though, the original reason for reading files eagerly (even for stat calls) was most likely to avoid inconsistencies when the file changes on the filesystem between stat and read. Is that correct, @arphaman? If so, I think I should abandon this patch.

Agreed; that consistency is important. I don't think stat should be decoupled from reading the file.

[arphaman]

In D114968#3168123, @dexonsmith wrote:

In D114968#3167519, @jansvoboda11 wrote:

Assuming the filesystem doesn't change during dependency scanning, this change keeps the consistency between stat/read calls for minimized files.

Thinking about it some more though, the original reason for reading files eagerly (even for stat calls) was most likely to avoid inconsistencies when the file changes on the filesystem between stat and read. Is that correct, @arphaman? If so, I think I should abandon this patch.

Agreed; that consistency is important. I don't think stat should be decoupled from reading the file.

Yes, exactly, it was to avoid the file system change inconsistencies. it should not be decoupled .

[jansvoboda11]

Thanks for confirming!

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

const OriginalContents *getOrCreateLocalOriginalContents(
       llvm::Optional<std::unique_lock<std::mutex>> &Lock);
 
   /// Get the minimized contents for \p Filename from local cache, populating it
   /// if necessary.
   const MinimizedContents *getOrCreateLocalMinimizedContents(
       StringRef Filename, llvm::sys::fs::UniqueID UID, SharedStat *&SharedStat,
       llvm::Optional<std::unique_lock<std::mutex>> &Lock);
 
+  /// The information requested from \c getOrCreateFileSystemEntry.
+  enum RequestedInformation {
+    RI_Status,
+    RI_Contents,
+  };
+
   /// Get the full filesystem entry for \p Path from local cache, populating it
   /// if necessary.
   llvm::ErrorOr<FileSystemEntryResult>
-  getOrCreateFileSystemEntry(StringRef Path);
+  getOrCreateFileSystemEntry(StringRef Path, RequestedInformation RI);
 
   /// 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

 withNewSize(const llvm::ErrorOr<llvm::vfs::Status> &Status, uint64_t NewSize) {
   if (!Status)
     return Status;
   return llvm::vfs::Status::copyWithNewSize(*Status, NewSize);
 }
 
 llvm::ErrorOr<FileSystemEntryResult>
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-    StringRef Filename) {
+    StringRef Filename, RequestedInformation RI) {
   SharedStat *SharedStat = nullptr;
   llvm::Optional<std::unique_lock<std::mutex>> GuardLock;
 
   const MaybeStat *LocalStat =
       getOrCreateLocalStat(Filename, SharedStat, GuardLock);
   if (!(*LocalStat))
     return LocalStat->getError();
   if ((*LocalStat)->isDirectory())
     return FileSystemEntryResult(*LocalStat, nullptr, nullptr);
 
   llvm::sys::fs::UniqueID UID = (*LocalStat)->getUniqueID();
 
   if (!shouldMinimize(Filename)) {
+    if (RI == RI_Status) {
+      // If the caller wants a status of a file that doesn't need minimization,
+      // reading it is not necessary.
+      return FileSystemEntryResult(*LocalStat, nullptr, nullptr);
+    }
+
     const OriginalContents *LocalOriginalContents =
         getOrCreateLocalOriginalContents(Filename, UID, SharedStat, GuardLock);
     return FileSystemEntryResult(*LocalStat, LocalOriginalContents, nullptr);
   }
 
   const MinimizedContents *LocalMinimizedContents =
       getOrCreateLocalMinimizedContents(Filename, UID, SharedStat, GuardLock);
   return FileSystemEntryResult(
       withNewSize(*LocalStat, LocalMinimizedContents->Contents.size()),
       &LocalMinimizedContents->Contents,
       &LocalMinimizedContents->PPSkippedRangeMapping);
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
 DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
-  auto Result = getOrCreateFileSystemEntry(Filename);
+  auto Result = getOrCreateFileSystemEntry(Filename, RI_Status);
   if (!Result)
     return Result.getError();
   return Result->Status;
 }
 
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MinimizedVFSFile::create(
   return llvm::ErrorOr<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);
 
-  auto Result = getOrCreateFileSystemEntry(Filename);
+  auto Result = getOrCreateFileSystemEntry(Filename, RI_Contents);
   if (!Result)
     return Result.getError();
   if (!Result->Status)
     return Result->Status.getError();
   return MinimizedVFSFile::create(Result.get(), PPSkipMappings);
 }