Skip to content

Conversation

@kiennq
Copy link
Member

@kiennq kiennq commented Jun 10, 2020

Since we're using lsp--fuzzy-score, there's no need for grouping candidates prefix anymore.

@yyoncho
Copy link
Member

yyoncho commented Jun 10, 2020

Can you do this PR against plists branch? We should focus on it otherwise it will never be merged.

@kiennq kiennq force-pushed the bug/dont-group-onprefix branch from 20e366c to 93b1111 Compare June 10, 2020 22:30
@kiennq kiennq changed the base branch from master to plists June 10, 2020 22:30
@kiennq
Copy link
Member Author

kiennq commented Jun 10, 2020

Done

@yyoncho yyoncho merged commit e699fce into emacs-lsp:plists Jun 11, 2020
(when (string-match fuz-query cand)
(setq cand (copy-sequence cand))
(put-text-property 0 1 'match-data (match-data) cand)
(put-text-property 0 1 'completion-score (lsp--fuzzy-score query cand) cand)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we calculate the completion-score here and remove the OR bellow? Also, access completion-score directly instead of using the get-text-property.

(plist-put fuz-queries start-point s))
s))))
(when (string-match fuz-query cand)
(setq cand (copy-sequence cand))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is needed? We should strive to reduce memory usage in this critical section. Note that dart language server might return 20k+ completion items.

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 modified the cached candidates to adding match-data and stuff so just copy the candidates to make sure.

(put-text-property 0 1 'completion-score (lsp--fuzzy-score query cand) cand)
cand)))
items))
(-sort (-on #'> (lambda (o)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should remove the usage of -sort in favour of sort in completion hotpath.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

(put-text-property 0 1 'completion-score (lsp--fuzzy-score query cand) cand)
cand)))
items))
(-sort (-on #'> (lambda (o)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-on would introduce some additional funcall - can we replace it with lambda?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@yyoncho
Copy link
Member

yyoncho commented Jun 11, 2020

And there is one more thing that we should do - use while-no-input when processing the completions. When it is hit we should return empty/previous completion result because it will be recalculated either way.

@kiennq
Copy link
Member Author

kiennq commented Jun 11, 2020

And there is one more thing that we should do - use while-no-input when processing the completions. When it is hit we should return empty/previous completion result because it will be recalculated either way.

Good point, I will do that

@yyoncho
Copy link
Member

yyoncho commented Jun 11, 2020

And there is one more thing that we should do - use while-no-input when processing the completions. When it is hit we should return empty/previous completion result because it will be recalculated either way.

Good point, I will do that

Hm, we should be careful, because this might not work, e. g. when user does company select next item we should not cancel the calculations...

yyoncho pushed a commit to yyoncho/lsp-mode that referenced this pull request Jun 11, 2020
kiennq added a commit to kiennq/lsp-mode that referenced this pull request Jun 13, 2020
@kiennq kiennq deleted the bug/dont-group-onprefix branch June 13, 2020 05:23
yyoncho pushed a commit to yyoncho/lsp-mode that referenced this pull request Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants