ioeric added a comment.
In D56903#1365487 <https://reviews.llvm.org/D56903#1365487>, @sammccall wrote:
> This looks pretty good! My main concern is latency (and variability of
> latency) in practice.
> Particularly:
>
> - we should avoid paying for fuzzyfind and fetching hundreds of results when
> we want exact-match
> - limit the damage in degenerate cases: should we limit to e.g. 5 index
> queries per TU?
>
> Actually, now that I think about it - if we can see a forward declaration,
> can't we do a lookup() instead of a fuzzyFind? Is the problem that this
> doesn't generalize to the no-declaration case (where it looks like a typo)?
I switched to use lookup requests for incomplete type diagnostics. And yes, the
typo errors need to use FuzzyFind or lookup by names as no USR is available.
> If we had an operation that looked like lookup() but worked by qname, would
> we design the feature the same way?
> In particular, would we want to batch the requests to the index (lookup
> takes a set of IDs)? This would affect the code structure a bit. But it would
> make the feature cost basically O(1) instead of O(errs)...
As chatted offline, we decided to leave the batching optimization as a TODO in
this patch. See inline comment for more detailed response regarding performance.
================
Comment at: clangd/ClangdUnit.h:91
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+ const SymbolIndex *Index);
----------------
sammccall wrote:
> Index is a reasonable (if surprising) param here, but I think we need to
> explicitly enable the include-fixing behavior. Even the very minimal
> hard-coded list of clang-tidy checks caused issues :-( And this is going to
> result in new network RPCs in some configs.
>
> I'd suggest keeping the Index param, and wrapping the "use include fixer?"
> policy along with the clang-tidy options in D55256 as some sort of
> "diagnostic options" struct. (Though be wary of name clash with the one in
> Diagnostics.h, sigh).
How about `ParseOptions`?
================
Comment at: clangd/IncludeFixer.cpp:39
+ const clang::Diagnostic &Info) const {
+ if (isIncompleteTypeDiag(Info.getID())) {
+ // Incomplete type diagnostics should have a QualType argument for the
----------------
sammccall wrote:
> can you add a trace to this function so we know what the impact on latency is?
Added a tracer in `fixInCompleteType` to avoid noise from unsupported
diagnostics.
================
Comment at: clangd/IncludeFixer.cpp:66
+ vlog("Trying to fix include for incomplete type {0}", IncompleteType);
+ FuzzyFindRequest Req;
+ Req.AnyScope = false;
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > limit?
> Why do we use fuzzyFind and not `lookup` here?
> Are there cases when we can't construct `SymbolID` for the `TagDecl`?
Switched to use lookup. The typo diagnostics (i.e. undefined symbol) can't use
lookup as there is no declaration, but we can definitely use lookup for
incomplete types in this patch.
================
Comment at: clangd/IncludeFixer.cpp:74
+ llvm::Optional<Symbol> Matched;
+ Index.fuzzyFind(Req, [&](const Symbol &Sym) {
+ // FIXME: support multiple matched symbols.
----------------
sammccall wrote:
> so issuing a bunch of fuzzy finds in sequence is clearly not ideal from a
> performance standpoint.
> Any ideas on what we might do better, or how we can limit the worst case?
> (not sure we need to do any better in this patch, but might be worth comments)
As you suggested, batching requests from all diagnostics would definitely help.
SymbolIndex already supports batch lookup by IDs, but we would need to extend
SymbolIndex to support lookup by Names for the typo errors that we are handling
in D57021. In order to batch index requests from all diagnostics, the existing
logic around handling and storing diagnostics might also need refactoring. I
added a TODO for this. We can revisit if the performance turned out to be a big
problem.
Another thing we could do is adding cache for index results. For example, for
the code copy-paste case, one incomplete/undefined symbol can cause multiple
errors. Caching should save us some unnecessary index queries.
For this patch, I added a limit on the number of index requests in IncludeFixer
(5 for now), which would prevent us from sending too many index requests when
building AST.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56903/new/
https://reviews.llvm.org/D56903
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
