[PATCH] D114966: [clang][deps] Split filesystem caches

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

dexonsmith added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:253-255
+  if (SharedOriginalContents.WasInserted)
+    SharedStat->Stat =
+        readFile(Filename, getUnderlyingFS(), SharedOriginalContents.Value);
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > Calling `readFile()` behind a lock doesn't seem great. I did confirm that the original code seems to do the same thing (lock outside of `createFilesystemEntry`), but this refactor seems to bake the pattern into a few more places.
> > 
> > When races aren't very likely it's usually cheaper to:
> > - lock to check cache, returning cached result if so
> > - without a lock, compute result
> > - lock to set cache, but if the cache has been filled in the meantime by another thread, return that and throw out the just-computed one
> > 
> > Maybe it'd be useful to add:
> > ```
> > std::atomic<bool> IsInitialized;
> > ```
> > to the MinimizedContents and OriginalContents structures stored in the shared cache. This could make it easier to decouple insertion in the shared cache from initialization. I.e., it'd be safe to release the lock while doing work; another thread won't think the default-constructed contents are correct.
> Could you expand on this a bit more? If we have a lock for each file, how is locking, reading, unlocking slower than locking, unlocking, reading, locking, unlocking?
You're right; if there's a lock per-file and all consumers want the result of all computations there's no benefit to releasing the lock quickly.
- If some consumers only want partial results (or already-computed results), can be faster to release quickly.
- Could be expensive to have mutexes per-file, since that's A LOT of mutexes. It might be cheaper in aggregate to switch to lock-free here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114966