This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement BOOST iterator
ClosedPublic

Authored by kbobyrev on Aug 20 2018, 6:05 AM.

Details

Summary

This patch introduces BOOST iterator - a substantial block for efficient and high-quality symbol retrieval. The concept of boosting allows performing computationally inexpensive scoring on the query side so that the final (expensive) scoring can only be applied on the items with the highest preliminary score while eliminating the need to score too many items.

Diff Detail

Event Timeline

kbobyrev created this revision.Aug 20 2018, 6:05 AM
kbobyrev planned changes to this revision.Aug 20 2018, 6:05 AM

This patch is in preview mode, documentation overhaul and minor cleanup incoming.

kbobyrev updated this revision to Diff 161477.Aug 20 2018, 7:20 AM
kbobyrev retitled this revision from [clangd] Introduce BOOST iterators to [clangd] Implement BOOST iterator.
kbobyrev edited the summary of this revision. (Show Details)

Add documentation, cleanup tests.

Looking at this one. (to make sure we don't clash with @ioeric)

clang-tools-extra/clangd/index/dex/Iterator.h
111 ↗(On Diff #161477)

Maybe make it constexpr and put the value into the header?

ilya-biryukov added inline comments.Aug 21 2018, 9:48 AM
clang-tools-extra/clangd/index/dex/Iterator.h
92 ↗(On Diff #161477)

Maybe add a note to the comment on why an ID parameter actually is here?
IIUC, we need to because various iterators in the tree may point to different elements and we need to know which one we've actually matched.

129 ↗(On Diff #161477)

Could we describe the rationale for keeping both consume and consumeAndBoost somewhere in the comments?

From the offline conversation, it seems consumeAndBoost is more expensive, but our clients will all use it at some point in the future.
The idea of paying for boosting without actually using it seems bad, so keeping this function separate makes sense.

164 ↗(On Diff #161477)

Maybe use float scores to align with the scoring code we have for completion?

kbobyrev updated this revision to Diff 161909.Aug 22 2018, 2:36 AM
kbobyrev marked 4 inline comments as done.
kbobyrev added a subscriber: sammccall.
  • Add more comments explaining the difference between consume() and consumeAndBoost() and their potential usecases for the clients
  • Move DEFAULT_BOOSTING_SCORE to the header by making it constexpr and change its type to float

Summarizing the offline discussion with @kbobyrev, @ioeric and @sammccall in two comments.

clang-tools-extra/clangd/index/dex/Iterator.h
96 ↗(On Diff #161909)

With limit iterator in mind, let's rename this to consume() and make it non-const.

137 ↗(On Diff #161909)

Let's remove this function and change the interface of consume to return a vector of pairs instead.

kbobyrev updated this revision to Diff 161936.Aug 22 2018, 5:59 AM
kbobyrev marked 2 inline comments as done.

Address remaining comments.

This revision is now accepted and ready to land.Aug 22 2018, 6:37 AM
This revision was automatically updated to reflect the committed changes.