dexonsmith added inline comments.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:161-162
+ const StringRef RawFilename) {
+ llvm::SmallString<256> Filename;
+ llvm::sys::path::native(RawFilename, Filename);
+
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > I'm a bit nervous about the impact of modifying the input filename on
> > Windows before passing it into other APIs. This could change behaviour of
> > lower layers of the VFS (since they'll see a different filename than when
> > DependencyScanningWOrkerFileSystem is NOT on top of them).
> >
> > Can we restrict this just to what's passed to IgnoredFiles? (Maybe add
> > `shouldIgnore()` API, which returns `false` if the set is empty, and then
> > locally converts to native and checks for membership...)
> >
> > It also seems wasteful to be calling `sys::path::native` and the memcpy all
> > the time, when usually it has no effect. Have you checked whether this
> > affects performance of scanning something big?
> Yeah, I can see that path changing between VFS layers can be problematic. I'm
> pretty sure we can get away with only converting `Filename` to its native
> form when interacting with `IgnoredFiles`.
>
> I haven't checked the performance impact. If it ends up being measurable, I
> could implement something like `sys::path::is_native` and avoid the copy most
> of the time on unix-like OSes. WDYT?
Probably it'll end up not being measurable, but if it is, something like
`is_native` might help... that said, if this will eventually be replaced with
logic relyin on fs::UniqueID it might not be worth optimizing.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:171-172
bool KeepOriginalSource = IgnoredFiles.count(Filename) ||
!shouldMinimize(Filename);
DependencyScanningFilesystemSharedCache::SharedFileSystemEntry
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > Looking at this, makes me wonder if this is just fixing a specific instance
> > of a more general problem.
> >
> > Maybe `IgnoredFiles` should be a set of `FileEntry`s instead of
> > `StringRef`s... but that'd create a different performance bottleneck when
> > the set is big, since creating the FileEntrys would be expensive. We'd want
> > the FileEntry lookup to be globally cached / etc. -- and FileManager isn't
> > quite safe to use globally.
> >
> > Do you think IgnoredFiles as-is will work well enough for where it'll be
> > used for PCH? Or do we need to catch headers referenced in two different
> > ways somehow?
> I think we could use `llvm::sys::fs::UniqueID` instead of the filename to
> refer to files. Since the VFS layer resolves symlinks when stat-ing a file,
> that should be a canonical file identifier. I can tackle that in a follow up
> patch.
Yup, a unique ID should work for a file identifier.
I'm concerned about the cost of looking up the unique ID — avoiding stat
traffic was measured to be an important performance benefit in the dependency
scanner model.
To avoid a perf regression, I think you could use caches like:
- ids: filename -> unique-id
- originals: unique-id -> original file content
- minimized: unique-id -> minimized file content
Where "ids" and "originals" are read/cached in lock-step when accessing a
filename, additionally computing "minimized" if not in the ignore-list. (Adding
a file to the ignore-list would put content in "ids" and "originals".)
The goal is to amortize the `stat` cost across the lifetime of the service
while ensuring a consistent view of the file content.
WDYT?
... regardless I think all of this is out of scope for the current patch, which
is still useful for unblocking adding tests to the subsequent patches in the
stack.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106064/new/
https://reviews.llvm.org/D106064
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
