Skip to content
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

Closed

Conversation

tompng
Copy link
Member

@tompng tompng commented Oct 17, 2024

Make symbol completion faster

  • Cache Symbol.all_symbols.sort and reuse it.
  • Restrict the number of symbol candidates to reduce completion/rendering cost in Reline.
  • Use bsearch to perform O(logN) search from cached sorted symbol list.
irb -f
irb(main):001> s='a';symbols = 100000.times.map{s=s.succ; [s.to_sym, ('a'+s).to_sym]};
irb(main):002> :aaabq
               :aaabk 
               :aaabl 
               :aaabm 
               :aaabn 
               :aaabo 
               :aaabp 
               :aaabq▄
               :azyd █
               :azye ▀
               :azyf  
               :azyg  
               :azyh  
               :azyi  
               :azyj  
               :azyk  

Completion candidates of symbol contains first 50 + last 50 symbols matched to the completion target.

@tompng tompng force-pushed the faster_symbol_completion_with_cache branch 2 times, most recently from b11e2dd to 326a56a Compare October 20, 2024 05:48

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)
Copy link
Member Author

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...

Copy link
Member

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.

@tompng tompng marked this pull request as ready for review October 20, 2024 06:05
Copy link
Member

@st0012 st0012 left a 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.

rescue EncodingError
# ignore
end.sort
start_index = symbol_names.bsearch_index { |sym| sym.to_s >= prefix }
Copy link
Member

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?

Copy link
Member Author

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)
Copy link
Member

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.

@tompng
Copy link
Member Author

tompng commented Oct 21, 2024

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 Symbol.all_symbols.

@tompng tompng force-pushed the faster_symbol_completion_with_cache branch from 326a56a to 97bd89a Compare October 21, 2024 18:15
@tompng tompng marked this pull request as draft October 25, 2024 06:24
@tompng
Copy link
Member Author

tompng commented Oct 27, 2024

In the following situation, Symbol.all_symbol.sort takes about a second. I think it is not acceptable.

Symbol.all_symbols.size is 100000 with average symbol length=15 (About the number in Rails app)
Running in docker (host and image platform mismatch)

@tompng tompng closed this Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants