[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 abandoned this revision.
jansvoboda11 added a comment.

Thanks for confirming!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114968/new/

https://reviews.llvm.org/D114968

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

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 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114968/new/

https://reviews.llvm.org/D114968

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114968/new/

https://reviews.llvm.org/D114968

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114968/new/

https://reviews.llvm.org/D114968

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 391370.
jansvoboda11 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114968/new/

https://reviews.llvm.org/D114968

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -290,7 +290,7 @@
 
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-StringRef Filename) {
+StringRef Filename, RequestedInformation RI) {
   SharedStat *SharedStat = nullptr;
   llvm::Optional> GuardLock;
 
@@ -304,6 +304,12 @@
   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);
@@ -321,7 +327,7 @@
 DependencyScanningWorkerFilesystem::status(const Twine ) {
   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;
@@ -381,7 +387,7 @@
   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)
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -225,10 +225,16 @@
   StringRef Filename, llvm::sys::fs::UniqueID UID, SharedStat *,
   llvm::Optional> );
 
+  /// 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
-  getOrCreateFileSystemEntry(StringRef Path);
+  getOrCreateFileSystemEntry(StringRef Path, RequestedInformation RI);
 
   /// The global cache shared between worker threads.
   DependencyScanningFilesystemSharedCache 


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -290,7 +290,7 @@
 
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-StringRef Filename) {
+StringRef Filename, RequestedInformation RI) {
   SharedStat *SharedStat = nullptr;
   llvm::Optional> GuardLock;
 
@@ -304,6 +304,12 @@
   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);
@@ -321,7 +327,7 @@
 DependencyScanningWorkerFilesystem::status(const Twine ) {
   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;
@@ -381,7 +387,7 @@
   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)
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

[PATCH] D114968: [clang][deps] Avoid reading file for stat calls

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Depends on D114966 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114968

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -290,7 +290,7 @@
 
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-StringRef Filename) {
+StringRef Filename, RequestedInformation RI) {
   SharedStat *SharedStat = nullptr;
   llvm::Optional> GuardLock;
 
@@ -304,6 +304,12 @@
   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);
@@ -321,7 +327,7 @@
 DependencyScanningWorkerFilesystem::status(const Twine ) {
   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;
@@ -381,7 +387,7 @@
   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)
Index: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===
--- 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -223,10 +223,16 @@
   StringRef Filename, llvm::sys::fs::UniqueID UID, SharedStat *,
   llvm::Optional> );
 
+  /// 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
-  getOrCreateFileSystemEntry(StringRef Path);
+  getOrCreateFileSystemEntry(StringRef Path, RequestedInformation RI);
 
   /// The global cache shared between worker threads.
   DependencyScanningFilesystemSharedCache 


Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -290,7 +290,7 @@
 
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-StringRef Filename) {
+StringRef Filename, RequestedInformation RI) {
   SharedStat *SharedStat = nullptr;
   llvm::Optional> GuardLock;
 
@@ -304,6 +304,12 @@
   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);
@@ -321,7 +327,7 @@
 DependencyScanningWorkerFilesystem::status(const Twine ) {
   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;
@@ -381,7 +387,7 @@
   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)
Index: