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

break: c rewrite #1270

Closed
wants to merge 5 commits into from
Closed

Conversation

Conni2461
Copy link
Member

@Conni2461 Conni2461 commented Sep 20, 2021

Nobody intends to rewrite telescope in c.

There is stuff missing (truncate) and we can play around with different data structures. And other ideas. This is basically a WIP / proof of concept branch.

benchmark script shows for max sorting 10k items (thats where this should help == scrolling):

Benchmark Group: 'lua vs c ffi' -----------------------
Benchmark #1: 'c ffi'
  Time(mean ± σ):    226.7 ms ±  27.7 ms
  Range(min … max):  187.3 ms … 262.0 ms  10 runs
Benchmark #2: 'lua'
  Time(mean ± σ):    1.3 s ± 181.6 ms
  Range(min … max):  1.1 s … 1.6 s  10 runs
Summary
  'c ffi' ran
  5.8 ± 1.1 times faster than 'lua'

Edit: Anyone who wants to test it. You need to run make in telescope directory.

underwhelming. Idk if it worth it.

Same speed with a debug build.
~1.5 times faster with a release build
@kkharji
Copy link
Member

kkharji commented Sep 20, 2021

PURE SPEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEDDDDDDDDDDD

@Conni2461 Conni2461 closed this Oct 4, 2021
@fdschmidt93
Copy link
Member

fdschmidt93 commented Oct 13, 2021

For the kicks, I've tried this branch and I must admit -- for the right use cases it's a landmark improvement.

The use case would be grep + (telescope-)fzf syntax. fzf.vim defaults to collect everything from rg with "" as a query and then just uses its algo to filter (see here). Our analogous picker I think would be builtin.grep_string { search = "" } which is quite notably slower in standard repos (eg telescope.nvim) on master but pretty much the same with this branch. (As a side note, sorting seems to have changed a bit unexpectedly though.)

I guess you've understandably closed this since it would incur a significant maintenance burden? Apologies if I lack the right knowledge here, a telescope-fzf-native writer job via nvim-telescope/telescope-fzf-native.nvim#32 with an updated https://github.com/nvim-telescope/telescope-fzf-writer.nvim would probably be the next best thing that is easier maintained? Just curious, I guess it's also fair to leave live_grep as it is though.

(I always am tempted to learn Rust and I think mlua might be cool here to speed up EntryManager maybe in more easily maintainable fashion? But riir and don't think we'd like to have a Rust module in telescope and no idea if it works easily with telescope-fzf-native)

@Conni2461
Copy link
Member Author

(As a side note, sorting seems to have changed a bit unexpectedly though.)

not everything was ported. Its a proof of concept branch thats also the reason why i closed it. Its not because it will be of a "significant maintenance burden".

TJ and I talked a lot about the future of telescope. Regarding a native component, fzf-native here and builtins. Basically none of it is public yet. But the gist is we will do a release and after that do breaking changes in a dev branch. Rust is probably the better call for telescope because its easier to build native lua modules with it. We dont need to force luajit because we use ffi and we dont need to require (or ship) the c lua headers so that people can build a native lib. Rusts package manager cargo handles this for us. And we can also offer a script that downloads the native component.

We can still pull in fzf-native into the repo and just use cffi inside rust, rather than inside lua.

I will share more of that roadmap and reasoning, soon

@fdschmidt93
Copy link
Member

fdschmidt93 commented Oct 13, 2021

Many thanks for elaborating! :) Sounds great and I must admit I've come around lately that we should separate core and builtins, so looking forward.

@TC72
Copy link
Contributor

TC72 commented Oct 25, 2021

I'm surprised to hear rust being talked about. I might be wrong but I thought the main languages for neovim were C for neovim and Lua for plugins. Isn't fzf-native a rewrite of core fzf from rust into C?

@fdschmidt93
Copy link
Member

Not that I know Rust (yet, I've been meaning to pick it up as my first lower level language.. so take with a grain of salt as I only know Python/Lua), it seems a bit more appealing than C for some parts of telescope core like the entry manager.

Rust has an expansive standard library and cargo is a great package manager that makes it easy to ship (maybe even precompiled?) libraries to users. In any case, cross-platform compilation should be very easy.

On how to expose rust in lua, see something like this

https://blog.kdheepak.com/loading-a-rust-library-as-a-lua-module-in-neovim.html

which levers

https://github.com/khvzak/mlua

Therefore, telescope can quite easily almost seamlessly tie in back-end components (like the entry manager) from Rust.

Isn't fzf-native a rewrite of core fzf from rust into C?

No, https://github.com/junegunn/fzf is written in highly optimized go, that is not easily optimized like that in Rust, see this discussion on fzf's rust equivalent skim.

I'll let others add more context (or please correct me) as this is just some high-level research I've done thinking about these sorts of issues.

@TC72
Copy link
Contributor

TC72 commented Oct 25, 2021

@fdschmidt93 thanks for explaining.

@fdschmidt93
Copy link
Member

I've actually started converting this to rust as part of my learning the language and my progress might be of interest to some:

https://github.com/fdschmidt93/telescope.nvim/blob/feat/rs-manager/src/lib.rs

The (super simple) implementation is pretty much as fast as the current linked list (same mean, much lower standard deviation, ie lua linked list might be faster/slower). What's interesting is that it scales super well to having a lot more entries (think 10K+ rather than 1K). But yeah, maybe a heap or linked list is better.

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.

4 participants