Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 33046 Build 33045: arc lint + arc unit
Event Timeline
This isn't quite ready for a thorough review, but I have a question for now: in BackgroundIndex::update, there is a step where we partition symbols and references into files.
For relations, should we include a copy in both the file containing the definition of the subject, and (if different) the file containing the definition of the object?
The point of sharding is to reduce duplication, so I would rather store it only in subject's definition location.
Ok, this is probably ready for a first round of review now.
I know I should add new test cases to at least DexTests and BackgroundIndexTests, I'll do this in the next version of the patch.
clang-tools-extra/clangd/index/Background.cpp | ||
---|---|---|
293 | nit: rename to SymbolIDToFile? | |
329 | nit: braces | |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
201 | redundant scope | |
223 | no need to pass RelationSlabs in here AllRelations are just value types. | |
247–248 | I think we need to pass relationslab in here. Since we might miss relations like the following that are outside the main file: class A {}; class B : public A {}; Would be glad if you could prove me right/wrong with a unittest as well. | |
clang-tools-extra/clangd/index/Index.h | ||
112 | We might need additional options, like limiting number of results. Could you instead accept a RelsRequest object? You can check RefsRequest for a sample | |
clang-tools-extra/clangd/index/MemIndex.cpp | ||
22–23 | + Relations.bytes() | |
24 | no need to put Relations into Data they are just values, right? | |
92 | nit: braces | |
clang-tools-extra/clangd/index/MemIndex.h | ||
29 | why not just store densemap<<s,p>,arrayref<o>> ? | |
clang-tools-extra/clangd/index/Merge.cpp | ||
126 | avoiding deduplicates is not enough, we also wouldn't want to report stale relations coming from static index. Could you check the logic in MergedIndex::refs, and hopefully refactor it into a class to share between these two? | |
clang-tools-extra/clangd/index/dex/Dex.cpp | ||
28–30 | +rels.size() | |
269 | nit: braces | |
clang-tools-extra/clangd/index/dex/Dex.h | ||
109 | let's store densemap<<s,p>, arrayref<o>> here as well |
clang-tools-extra/clangd/index/Index.h | ||
---|---|---|
112 | I called it RelationsRequest to avoid visual confusion with RefsRequest. | |
clang-tools-extra/clangd/index/MemIndex.h | ||
29 | I used std::vector<o> because it wasn't clear where the array would live otherwise. | |
clang-tools-extra/clangd/index/Merge.cpp | ||
126 | The description in MergedIndex::refs() says: // We don't want duplicated refs from the static/dynamic indexes, // and we can't reliably deduplicate them because offsets may differ slightly. // We consider the dynamic index authoritative and report all its refs, // and only report static index refs from other files. It seems to me that the problem of "can't reliably deduplicate them because offsets may differ slightly" doesn't apply to relations, as there are no offsets involved. So, is there still a need to have additional logic here? If so, what would the logic be -- don't return an object O1 from the static index if we've returned an object O2 from the dynamic index defined in the same file as O1? (Note that to implement this, we'd have to additionally look up each SymbolID we return in the symbol table, as we don't otherwise know what file the object is located in.) | |
clang-tools-extra/clangd/index/dex/Dex.cpp | ||
28–30 | Size is only the backing data size, so if we don't include the relations in the backing data, we shouldn't include Rels.size(), right? |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
202 | nit: braces | |
clang-tools-extra/clangd/index/Index.h | ||
76 | let's have a limit as well | |
112 | yeah that's a good call thanks! | |
139 | thinking about this callback, I don't think there is any user that will make use of the SymbolIDs without querying for a Symbol. So to reduce this boilerplate let's rather move this lookup into this API(all SymbolIndex implementations provide lookup anyway so they all have a way to convert a SymbolID into a Symbol) and change the callback to accept a const Symbol& instead of a SymbolID | |
clang-tools-extra/clangd/index/Merge.cpp | ||
126 | yes I was referring to second part actually, but you are right we have no good way of distinguishing stale information in this case. Let's leave it as it is, but add a comment stating that we might return stale relations from static index. | |
clang-tools-extra/clangd/index/dex/Dex.cpp | ||
28–30 | ok, but then let's make sure we include size of the densemap in estimateMemoryUsage manually. | |
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp | ||
220 | could we instead check only relation is A_CC isbaseof B_CC ? |
Address remaining review comments
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
247–248 | You are right! Fixed, and added a test to FileIndexTests. |
LGTM, except the batch query support.
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
94 | we should have only a couple call sites to this function. Let's rather change those instead of introducing a new helper. | |
clang-tools-extra/clangd/index/Index.h | ||
77 | sorry for missing it in previous iteration. but this should also enable batch queries. let's make this one a llvm::DenseMap<SymbolID>. We have a network based index(internally) and it uses this batch query optimization to get rid of network latency. | |
111 | I think the old comment is better. No need to expose internals. | |
clang-tools-extra/clangd/index/dex/Dex.cpp | ||
244 | could you revert these changes? | |
268 | can you also add a span trace::Span Tracer("Dex relations"); |
clang-tools-extra/clangd/index/Index.h | ||
---|---|---|
77 | What is an example use case for a batch query? In a typical LSP editor, the user can only request a type hierarchy for one class at a time. |
clang-tools-extra/clangd/index/Index.h | ||
---|---|---|
77 | yes, that's not really applicable to TypeHierarchy, but we are rather introducing another endpoint to an existing API(SymbolIndex), which will be used by a lot more clients later on. The first example that comes to my mind is, fetching all overrides of a class' virtual methods for example. |
clang-tools-extra/clangd/index/dex/Dex.cpp | ||
---|---|---|
244 | Reverted. Not sure how they were introduced in the first place... rebase error perhaps. |
This seems to have broken gcc 5.4 builds -- for example see http://lab.llvm.org:8011/builders/clang-cmake-armv7-lnt/builds/29/steps/build%20stage%201/logs/stdio.
I believe the following patch should fix the problem:
diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp index a90c51b0990..802e4c4a396 100644 --- a/clang-tools-extra/clangd/index/FileIndex.cpp +++ b/clang-tools-extra/clangd/index/FileIndex.cpp @@ -72,7 +72,7 @@ static SlabTuple indexSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP, " relations slab: {7} relations, {8} bytes", FileName, IsIndexMainAST, Syms.size(), Syms.bytes(), Refs.size(), Refs.numRefs(), Refs.bytes(), Relations.size(), Relations.bytes()); - return {std::move(Syms), std::move(Refs), std::move(Relations)}; + return std::make_tuple(std::move(Syms), std::move(Refs), std::move(Relations)); } SlabTuple indexMainDecls(ParsedAST &AST) {
Fixed in r363504.
nit: rename to SymbolIDToFile?