Page MenuHomePhabricator

[clangd] BackgroundIndex rebuilds symbol index periodically.
ClosedPublic

Authored by ioeric on Dec 17 2018, 8:21 AM.

Details

Summary

Currently, background index rebuilds symbol index on every indexed file,
which can be inefficient. This patch makes it only rebuild symbol index periodically.
As the rebuild no longer happens too often, we could also build more efficient
dex index.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Dec 17 2018, 8:21 AM
kadircet added inline comments.Dec 17 2018, 9:42 AM
clangd/index/Background.cpp
464 ↗(On Diff #178471)

why not simply check if BuildIndexPeriodMs has passed and issue rebuild here? That way we won't spawn an additional thread, and get rid of the CV

(from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexing for multiple changes in rapid succession (such as hitting the "save all" button in some text editor - or having made a few other quick changes one after another maybe), but still provides reasonable up-to-date data for the user.

(but I don't know anything about clangd & maybe what I'm saying makes no sense/isn't relevant)

ioeric marked an inline comment as done.Dec 18 2018, 2:09 AM

(from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexing for multiple changes in rapid succession (such as hitting the "save all" button in some text editor - or having made a few other quick changes one after another maybe), but still provides reasonable up-to-date data for the user.

(but I don't know anything about clangd & maybe what I'm saying makes no sense/isn't relevant)

Thanks for the input David.

The term indexing in clangd is getting bit overloaded :( There are two kinds of "indexing " happening in the background index: 1) parse and collect symbols from the TU and 2) build a symbol index (e.g. for fast fuzzy search. this is the real index!) for the symbols. (1) happens in a manner similar to what you described (e.g. we don't reindex files with the same hash). This patch addresses (2). As there can be multiple threads collecting symbols concurrently, it's not ideal to rebuild the symbol index (data structure) for each index action; thus, we are making the index rebuild only run periodically.

Happen to chat more and provide more details if you are interested.

clangd/index/Background.cpp
464 ↗(On Diff #178471)

As chatted offline, the main problem is that the last index action might trigger an index update if it finishes shortly after an index rebuild; there is not trivial way for a index worker to figure out that it's the last run. Also the testing seems to be harder with this approach.

kadircet added inline comments.Dec 18 2018, 4:59 AM
clangd/index/Background.h
112 ↗(On Diff #178471)

We already have a mutex and cv, maybe get rid of this one signal the CV whenever we have an update and sleep for buildindexperiodms before issuing the re-build?

ioeric marked an inline comment as done.Dec 18 2018, 6:27 AM
ioeric added inline comments.
clangd/index/Background.h
112 ↗(On Diff #178471)

IndexCV serves two purposes: 1) get notified when ShouldStop is set and 2) timeout after BuildIndexPeriodMs. We wouldn't want to sleep here because it can take too long to shutdown clangd if BuildIndexPeriodMs is big.

kadircet added inline comments.Dec 18 2018, 7:05 AM
clangd/index/Background.cpp
352 ↗(On Diff #178471)

Is double checking really necessary? I suppose it is for the case that we miss the notification, if that's the case maybe put a comment?

clangd/index/Background.h
112 ↗(On Diff #178471)

by sleep I still meant IndexCV.wait_for

kadircet added inline comments.Dec 18 2018, 7:11 AM
clangd/index/Background.h
112 ↗(On Diff #178471)

well actually I believe we should use wait_until rathert than wait_for, since we don't wanna stop waiting if we weren't stopped.

kadircet accepted this revision.Dec 18 2018, 7:33 AM

LGTM, thanks!

clangd/index/Background.h
112 ↗(On Diff #178471)

As discussed offline, even with wait_until we still need the boolean flag to check if anything was updated.

This revision is now accepted and ready to land.Dec 18 2018, 7:33 AM
ioeric updated this revision to Diff 178676.Dec 18 2018, 7:39 AM
ioeric marked an inline comment as done.
  • Add comment about double-check of ShouldStop.
This revision was automatically updated to reflect the committed changes.