-
Notifications
You must be signed in to change notification settings - Fork 119
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache symbol completion result #1021
Conversation
b11e2dd
to
326a56a
Compare
|
||
commands | result.completion_candidates.map { target + _1 } | ||
analyze_result = result.instance_variable_get(:@analyze_result) | ||
if analyze_result.is_a?(Array) && analyze_result[0] == :symbol && analyze_result[1].is_a?(String) |
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 think it is relatively safe because it checks the value strictly, but it's a bit hacky...
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.
Tbh I'd prefer updating repl_type_completor
gem to have a better interface for this. And we can print warnings to ask users to upgrade to that version.
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 feel sorry to say this because I was the one who brought up this idea 😅 But are we sure that the performance improvement is worth the complexity? Especially the binary search part.
lib/irb/completion.rb
Outdated
rescue EncodingError | ||
# ignore | ||
end.sort | ||
start_index = symbol_names.bsearch_index { |sym| sym.to_s >= prefix } |
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.
Would it be more efficient if we convert the prefix to symbol rather than calling to_s
on all iterated symbols?
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.
Sorry symbol_names
was already a string. I deleted to_s
and add missing return [] unless start_index
.
We need to search from an array of string symbols.map(&:inspect)
to exclude symbols like :"a b"
.
|
||
commands | result.completion_candidates.map { target + _1 } | ||
analyze_result = result.instance_variable_get(:@analyze_result) | ||
if analyze_result.is_a?(Array) && analyze_result[0] == :symbol && analyze_result[1].is_a?(String) |
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.
Tbh I'd prefer updating repl_type_completor
gem to have a better interface for this. And we can print warnings to ask users to upgrade to that version.
I think there is room for discussion whether to keep symbol completion or to drop it. If we're going to keep it, we need to do this bsearch because symbol filtering (main bottleneck in IRB side) is few times slower than |
326a56a
to
97bd89a
Compare
In the following situation, Symbol.all_symbols.size is 100000 with average symbol length=15 (About the number in Rails app) |
Make symbol completion faster
Symbol.all_symbols.sort
and reuse it.Completion candidates of symbol contains first 50 + last 50 symbols matched to the completion target.