qchateau added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1843
+ auto Kind = Callee.SymInfo.Kind;
+ if (Kind != SK::Function && Kind != SK::InstanceMethod &&
+ Kind != SK::ClassMethod && Kind != SK::StaticMethod &&
----------------
nridge wrote:
> If memory usage of `Dex::RevRefs` becomes a concern, we could consider only
> storing the references that would pass this filter in the first place. That
> would trade off time spent building the reverse lookup (we'd have to do
> symbol lookups or keep around extra state to track the symbol kinds) for
> space savings.
I've used tried this patch with the whole llvm project indexed. If I remember
correctly it's something in the like of 100MB, around 5-10% même usage of
clangd. As such it's not very high but it's definitely not negligible,
especially for a feature that's not used very often.
================
Comment at: clang-tools-extra/clangd/index/Index.h:85
+struct RefersToResult {
+ /// The source location where the symbol is named.
----------------
nridge wrote:
> As the protocol wants the outgoing calls grouped by symbol, we could consider
> storing them (and having the `SymbolIndex` API expose them) in that way.
>
> The main motivation would be space savings on the storage side
> (`Dex::RevRefs`), as in the case of multiple calls to the same function we
> only need to store the target `SymbolID` once.
>
> However, it may not be worth doing this, as having a large number of outgoing
> calls to the same target inside a given function is likely to be rare, and
> vectors have their own overhead. (It's also not clear to what extent the
> details of the LSP protocol should dictate the shape of the `SymbolIndex`
> API.)
I indeed believe that I'd use more memory than it would save, but I did not try
it.
Anyway I think the difference would be minimal, and I'd say we should choose
what gives the "best" API
================
Comment at: clang-tools-extra/clangd/index/Index.h:133
+ virtual bool
+ refersTo(const RefsRequest &Req,
+ llvm::function_ref<void(const RefersToResult &)> Callback) const =
0;
----------------
nridge wrote:
> Perhaps we should have a distinct request structure, for future extensibility?
Sure, that makes sense
================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:97
+ Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
+ for (const auto &Pair : Refs) {
+ for (const auto &R : Pair.second) {
----------------
nridge wrote:
> Looping over all refs seems expensive even for `MemIndex`. Perhaps we should
> have a reverse lookup table similar to `RevRefs` here as well?
That's pretty fast even on my laptop IIRC, and only used for outgoing calls
atm.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93829/new/
https://reviews.llvm.org/D93829
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
