New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign SymbolStore and SymbolProvider #672
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit c164fc1.
This allows us to skip a lot of symbols, thus making the iteration way faster. Moreover, we can keep splicing into the SymbolStore fast because as soon as lines get removed, the index is automatically kept in-sync (e.g. because it's stored per each line). On a file with ~ 26k lines, autocompleting a word takes < 80ms.
| {Selector} = require 'selector-kit' | ||
| SymbolStore = require './symbol-store' | ||
|
|
||
| # TODO: remove this when `onDidStopChanging(changes)` ships on stable. | ||
| bufferSupportsStopChanging = -> typeof TextBuffer::onDidChangeText is "function" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am testing for the presence of onDidChangeText here because I couldn't find any other efficient heuristic to determine whether or not atom/text-buffer#126 shipped. This is temporary, though, so it might not be that important.
as-cii
added a commit
that referenced
this pull request
Feb 21, 2016
Redesign SymbolStore and SymbolProvider
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.


This PR changes
SymbolStoreandSymbolProviderso that they're faster when retrieving suggestions and when computing symbols.Before
The way those two objects previously worked relied on storing symbols and the buffer rows they were contained in: this was a problem in a number of ways, especially after buffer changes where we had to scan through all those symbols to adjust their position according to the aforementioned change (in addition, we were doing this synchronously, blocking the UI thread until we scanned all the symbols).
(
~ 350ms, benchmarks performed by writing 3 letters with 500 cursors)Also, searching into
SymbolStoreinvolved a series of queries toselectorMatchesScopeChainwhich caused some major slowdowns every timeSymbolProvider.prototype.getSuggestions()got called.(
~ 150ms, benchmarks performed by searching forcorin benchmark/large.js)After
With this PR we're changing the way we store symbols (avoiding to index them by buffer row) and computing/scoring suggestions (e.g. locality score, fuzzy score, etc.) as we traverse through lines. This allows us to perform a very minimal bookkeeping on each buffer change while still being fast when retrieving possible autocompletions. Moreover, we now make use of
TextBuffer.prototype.onDidStopChanging, so that writing one or ten letters in a row doesn't cause the main thread to stall.This is how symbols re-computation looks like after these changes:
(馃悗 )
~ 46ms, 8x fasterWhen storing tokens, we keep an index of symbols by letter for each buffer row. When retrieving them, we exploit the fact that a symbol needs to start with the
prefix's first letter and, therefore, we score only tokens that match that first character.(馃悗 )
~ 53ms, 3x fasterStoring that index on a per-row basis has the advantage of not requiring us to deal with the "moving nature" of buffer rows, but forces us to iterate over every single line. Ideally here we may want to build a smarter data structure (i.e. a tree) that indexes letters across rows; on the other hand, though, the array-based implementation has pretty decent performance, and I believe we can avoid the added complexity of introducing an extra data structure for the time being.
Conclusion
Overall, this should allow us to take autocomplete-plus out of the multi-cursor slowness equation. I still feel like there's room for improvement (e.g. see the across rows index mentioned in the previous section) but I think this should allow us to move forward and deal with other bottlenecks in packages and tokenization.
/cc: @nathansobo @maxbrunsfeld @benogle for馃憖