bnbarham added a comment.
> You're correct that this overhead has been measured on implicit module
> builds. As I mentioned in the commit message this saves over 20% of the
> overall built time in some cases. This time is split between module
> validation (which could be skipped) and HeaderSearch (which cannot be
> skipped).
Heh, I read the commit as saying we've seen a 20% overhead, but not that this
completely removed that. Not sure why I didn't make that connection in my head,
it's a reasonable implication. Very nice!
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:5115-5130
+ for (const auto &File : CI.getHeaderSearchOpts().VFSStatCacheFiles) {
+ llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
+ llvm::MemoryBuffer::getFile(File);
+ if (!Buffer) {
+ Diags.Report(diag::err_missing_vfs_stat_cache_file) << File;
+ continue;
+ }
----------------
friss wrote:
> bnbarham wrote:
> > IMO VFS overlays being modeled like this is a mistake and makes
> > reasoning/testing about them fairly difficult. I have a PR up
> > https://reviews.llvm.org/D121423 to change `OverlayFileSystem` to make more
> > sense and be a real overlay rather than what it is now. If I finish that
> > one off, how would you feel about changing the behavior of
> > `StatCacheFileSystem` to just immediately error if it doesn't contain the
> > file, rather than proxy (and thus chain) as it does now?
> >
> > So for multiple files we'd then have:
> > - OverlayFileSystem
> > - StatCacheFileSystem
> > - StatCacheFileSystem
> > - RealFileSystem
> >
> > Then any non-stat or exists call would return
> > `llvm::errc::no_such_file_or_directory` and then the next FS would be used
> > instead.
> >
> > I don't think this *really* matters for `StatCacheFileSystem`, so I'm fine
> > if you'd rather not wait for me to change `OverlayFileSystem`. I can make
> > the changes myself after getting my PR in.
> I don't think that's really doable if you want to keep the ability to cache
> negative hits. If a miss always results in a query to the real filesystem,
> then you're not saving the `stat` call. A lot of the time is spent in
> HeaderSearch which queries a similar number of non-existing and existing
> files.
> But I'm not dead set on this. I also haven't spent a lot of time thinking
> about your proposal.
That's a great point. It'd be easy enough to avoid by eg. passing down an error
to use for either "this is a path I handle and it doesn't exist" or "this is
not a path I handle" and handling that in `OverlayFileSystem`, which could
default to `llvm:errc::no_such_file_or_directory`. Generally I just think this
is easier to reason about, but it's more problem for `RedirectingFileSystem`
then it is for this (since this isn't *really* an overlay at all).
This is definitely fine as is either way though. If I find
`RedirectingFileSystem` needs the same sort of idea then we can revisit it then.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2959-2960
+ // the pathis different. The canonicalization that the call to
remove_dots()
+ // does leaves only '..' with symlinks as a source of confusion. If the
path
+ // does not contain '..' we can safely say it doesn't exist.
+ if (std::find(sys::path::begin(SuffixPath), sys::path::end(SuffixPath),
----------------
friss wrote:
> bnbarham wrote:
> > This sentence is a little confusing to me. `remove_dots` just leaves `..`
> > unless you pass `remove_dot_dot` (but it doesn't do any checking). IMO just
> > the `If the path does not contain '..' we can safely say it doesn't exist.`
> > is enough.
> `remove_dots` does more than remove dots. It is confusing, but it also
> removes excess separators (see the `Canonical` unit test). This means that
> the cache will work for /path/to/cache/file/a as well as
> /path/to/cache/file///a and /path/to/cache/file/././a. There are basically
> infinite spellings of a path just by adding `.` and additional separators.
> `..` is interesting because it's not semantically preserving to remove them
> in the presence of symlinks.
> I'm fine with simplifying the description, but that is the nuance I tried to
> convey.
I suppose it could be confusing if you didn't know what `remove_dots` did, but
in that case I'd prefer something like:
> `remove_dots` canonicalizes the path by removing `.` and excess separators,
> but leaves `..` since it isn't semantically preserving to remove them.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2961
+ // does not contain '..' we can safely say it doesn't exist.
+ if (std::find(sys::path::begin(SuffixPath), sys::path::end(SuffixPath),
+ "..") == sys::path::end(SuffixPath)) {
----------------
friss wrote:
> bnbarham wrote:
> > FWIW `StringRef` has a `contains`
> But that wouldn't be correct. Here we are looking for a path component which
> is `..`. A simple text search would fire on a filename containing `..`. I
> think this search on components is the only correct way to do this.
Ah yeah, fair enough 👍
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136651/new/
https://reviews.llvm.org/D136651
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
