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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
