ilya-biryukov added inline comments.
================
Comment at: clangd/URI.h:131
+/// Resolves URI to file paths with cache.
+class URIToFileCache {
+public:
----------------
Maybe move it into the `Backgroud.cpp`, e.g. as a private member of
`BackoundIndex`?
We don't have other use-case for it to the date and it doesn't really good like
a good public API at this point, i.e. lacking docs on what it does and why it's
useful, which are useful if we move it into a public header.
================
Comment at: clangd/index/Background.cpp:312
+ // Skip if file is already up to date.
+ auto DigestIt = IndexedFileDigests.try_emplace(Path);
+ if (!DigestIt.second && DigestIt.first->second == Hash)
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > We still update digests and symbols non-atomically in that case. Could we
> > do both under the same lock, similar to how it's done in `loadShards`?
> Moved update to the end of the loop, but now we might perform unnecessary
> work for building {symbol,ref}slabs for files that are already up-to-date. It
> shouldn't be too much of an issue, as a solution we can lock at the entrance
> and only check if the file was up-to-date, than we update at the exit. Or we
> can keep a snapshot.
LG, thanks. I don't think this extra work should pop up in the profiler, so
we're good.
================
Comment at: clangd/index/Background.cpp:311
+ "Trying to update symbols for a file that hasn't been indexed");
+ // If we this is not the latest version of the file just skip.
+ if (LatestFileDigests[Path] != Hash)
----------------
NIT: s/we this/this/
================
Comment at: clangd/index/Background.cpp:312
+ // If we this is not the latest version of the file just skip.
+ if (LatestFileDigests[Path] != Hash)
+ continue;
----------------
An alternative strategy is to also update if the `IndexedFileDigests` does not
contain a value.
That would ensure we populate **some** version of all files as fast as
possible, but only update for the latest version. To me personally this looks
like a reasonable right trade-off (and would require updating just a few lines
of code)
================
Comment at: clangd/index/Background.cpp:319
+ DigestIt.first->second = Hash;
+ vlog("Update symbols in {0}", Path);
+ IndexedSymbols.update(Path, std::move(SS), std::move(RS));
----------------
This might be too verbose even for `vlog()`, i.e. this would produce thousands
of messages for each TU. Maybe omit it?
================
Comment at: clangd/index/Background.cpp:435
+ for (const auto &P : LatestDigests)
+ LatestFileDigests[P.first] = P.second;
}
----------------
I believe we want to avoid updating the digests for the dependencies if the
digest for the main file has changed in the meantime.
Otherwise, we might record the stale versions of the dependency digests for the
rest of the index lifetime.
Note that this might invalidate the assertion in `update() ` that checks all
files have digests.
================
Comment at: clangd/index/Background.cpp:586
+ }
+ // FIXME: this should rebuild once-in-a-while, not after every file.
+ // At that point we should use Dex, too.
----------------
Didn't we land the periodic rebuilds? Should this be obsolete now?
================
Comment at: clangd/index/Background.h:117
llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
+ llvm::StringMap<FileDigest> LatestFileDigests; // Key is absolute file path.
std::mutex DigestsMu;
----------------
NIT: maybe add a comment that we use this to decide which of the concurrently
running indexing operations should take precedence?
================
Comment at: clangd/index/SymbolCollector.cpp:213
bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
- const auto& SM = ND.getASTContext().getSourceManager();
+ const auto &SM = ND.getASTContext().getSourceManager();
return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
----------------
NIT: look unrelated, maybe land as a separate NFC commit right away?
Not a big deal, though, feel free to ignore
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55224/new/
https://reviews.llvm.org/D55224
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
