perf(source): only filter up to 200 entries and dont use the cache #1574
Conversation
| end | ||
| end | ||
| end | ||
| self.cache:set({ 'get_entries', tostring(self.revision), ctx.cursor_before_line }, entries) |
There was a problem hiding this comment.
I think get_entries cache will reduce the CPU cost. Why do you remove this?
There was a problem hiding this comment.
It leads to big memory issues with tailwind and probably others.
There was a problem hiding this comment.
I'll see if I can incorporate the cache again in some other way that doesn't include all the memory overhead
There was a problem hiding this comment.
Currently, nvim-cmp will save the all of caches for each user input. (f -> fo -> foo).
It can be helped for <BS> but I think we can avoid it (The <BS> use-case can be ignored).
There was a problem hiding this comment.
I just added caching back, but only if the filtered entries list is < max_item_count.
It's only then that we can re-use the cache, otherwise we might miss entries that could have matched, but were not included.
This also still fixes the memory issue since we won't store too big entry arrays in the cache
There was a problem hiding this comment.
I believe this solution is actually optimal. When the entries are huge, we filter and break as soon as we have 200 matches.
As soon as we filter down to less than 200 matches, the cache is used and the big entries list no longer needs to be fully processed.
There was a problem hiding this comment.
I agree that this fix improves the performance.
It also makes sense that the current nvim-cmp is already an incorrect implementation, so there is no negative behavior.
However, I doubt that this approach is the right way.
That being said, I'm not planning any drastic improvements to nvim-cmp, so I agree with efficiency first.
| end)() | ||
| if self.filtered.ctx and self.filtered.ctx.id == ctx.id then | ||
| return self.filtered.entries | ||
| end |
|
Just my little thought: current implantation of limiting entries without pre-sorting is wrong and leads to suboptimal result candidates and early break of filtering seems to conflict with this. In the theory, all the entires should be done with filtering to yield the best candidates. However, by early break we'd gain huge performance improvement because the current fuzzy matching implementation is not efficient enough for some extreme cases. This might be a choice between correctness and performance. |
|
@yioneko the current cmp implementation already does pre-filtering and only sorts the resulting entries. But I do agree, that it would be better to include your changes as well, so that we can remove that 200 limit. Maybe you should also open a PR with your async changes? |
|
@folke What I meant is that sorting should be done before limiting to preserve entries with higher expectation, and this actually conflict with early break of breaking by
My changes are pretty hacky and should not be considered for merging. Also I don't have much time to work on formalizing it currently. (Would like to if I have time, I also have some other ideas for optimization) |
|
@folke I can merge this for now. Do you have any work left to do? |
|
Good to go! I'll work on an async PR after this to further optimize the code. |
|
Thank you~! |
…indow * upstream/main: (31 commits) fix entry highlight in complete-menu (hrsh7th#1593) Remove max_item_count from source configuration feat: cmp async (hrsh7th#1583) ghost text inline (hrsh7th#1588) Fix demo video in README.md (hrsh7th#1585) docs: fix adjecent typo (hrsh7th#1577) docs: fix typos, add confirm.behavior example cfg (hrsh7th#1576) perf(source): only filter up to 200 entries and dont use the cache (hrsh7th#1574) fix(ghost_text): safely apply virtual_text highlight (hrsh7th#1563) Improve misc.merge Fix hrsh7th#897 Added a modified=false to documentation buffer, so it can be removed without E89 errors (hrsh7th#1557) Fix hrsh7th#1556 fix 1533, add regression test (hrsh7th#1558) Add `buftype=nofile` for entries_win and docs_win - reddit user mention this... Fix hrsh7th#1550 Format with stylua Add test for hrsh7th#1552 Close hrsh7th#1552 Revert hrsh7th#1534 temporaly fix typo (hrsh7th#1551) ...
This PR changes the way
get_entriesworks, but semantically it's still the same as before: