[PATCH] D114966: [clang][deps] Split stat and file content caches

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:132-134
+  /// Non-owning pointer to the file contents. This can be shared between
+  /// multiple entries (e.g. between a symlink and its target).
+  std::atomic<CachedFileContents *> ContentsAccess;
----------------
dexonsmith wrote:
> This comment is out of date, since there's a 1:1 correspondence between CachedFileSystemEntry and CachedFileContents since they're both looked up by UID.
> 
> Also, `ContentsAccess` is never modified after construction so it can just be a raw pointer.
> 
> Outlining the allocation of `CachedFileContents` might still make sense, since it's big and stat failures (with no content) are probably the common case due to header search patterns... only if we actually create these for stat failures though. Might be worth a comment saying why it's outlined. Maybe it should even be delayed to a follow-up commit to simplify this patch, since now that CachedFileSystemEntry is per-UID it doesn't seem to be required here... but the fields probably need to be made `mutable` regardless somehow so it doesn't seem like a ton of churn.
I've updated the comment and changed `ContentsAccess` to a raw pointer.

I agree outlining `CachedFileContents` in a follow-up patch would be cleaner, but I don't have much time to spare on prettifying git history at this moment unfortunately.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:160-161
+
+    /// Map from unique IDs to cached contents.
+    llvm::DenseMap<llvm::sys::fs::UniqueID, CachedFileContents *> ContentsCache;
+
----------------
dexonsmith wrote:
> I think this map can be deleted, since it's not actually used to deduplicate/share anything that `EntriesByUID` doesn't handle.
Yeah, that's right. Removed this in the latest revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114966