modules/performance: add an option to combine plugins to a single plugin pack

[stasjok]

Hey. I'm the one from this comment in [Feature] Lazy loading issue. I decided to give nixvim a try, but since I don't want to lose my performance optimizations, I implemented them in nixvim. Since my comment have got a couple of upvotes, here I am with a PR. I don't know if this would be merged, since this feature have caveats. But if it works for your config, you'll get a free performance boost (startup time and every time you :e a buffer). It's optional, so it shouldn't brake existing configs. I still think that this is more important than trying to lazy load everything (but for some specific cases lazy loading is also useful).

I'm planning to implement three performance options:

1. This one: combine plugins into a single pack
2. Optimize runtimepath further (basically set rtp to essential directories, removing everything else, mostly all system dirs). On my system with home-manager I have 14 useless items there. It's not yet implemented, but should be easy. For now I still have this in my config: https://github.com/stasjok/dotfiles/blob/36037f523185ba1409dd953999fda0f0db0dbd4f/nvim/init.lua#L1-L17, so I'm not in a hurry. It would depend on a feedback on this PR.
3. Byte compile every lua file. It's already implemented, will be a second PR.

To get you interested here are startuptime measurements (all measurements are taken from my config after a couple of tries with a hot disk cache):

1. No optimizations: 155 ms
2. All plugins combined: 95-100 ms
3. Everything byte compiled (including plugins and nvim runtime): 55-57 ms
4. rtp optimized: 53-55 ms (doesn't do much, to be honest)
5. For comparison, vim.loader.enable() without everything from the above: 115-120 ms

This PR adds a performance.combinePlugins option with two tunables: pathsToLink for choosing what paths are linked to plugin pack and standalonePlugins for specifying what plugins to exclude from combining. Last option is important, because for combining to work there must not be any file and doc tag collisions. Good news is if it builds, most of the time it would work correctly (except when some paths need to be added to pathsToLink).

Also this PR adds a couple of examples on how to fix errors in nixvim on a per plugin case (so most of the time user wouldn't need to configure anything). This is not all problematic plugins, but the ones I have in my config (to get this PR usable). Bad news came from a recent nixpkgs PR NixOS/nixpkgs#321550. After that PR parser queries are colliding with treesitter plugins. It can be fixed in user overlay (example), or like in this PR with nvim-treesitter (to exclude plugins containing a collection of queries from combining), or meta.priority can also be used. Because of this issue I even wanted to introduce a second layer of combined plugins, but for now a completely separate plugins would suffice. If the list of such exclusions will be too large, than it can be reevaluated.

[stasjok]

Can someone tell me what test is failing? For me evaluation takes too much RAM for my VM, so nix is getting killed by oom-killer.

[stasjok]

Am I wrong thinking that buildbot instances also have not enough memory? All this strange unexpected end-of-file errors, looks like nix can't finish. I've added a pretty extensive testing for options here, but it still only 10 new tests.

[traxys]

We have some issues currently with buildbot :/

[MattSturgeon]

This looks interesting! Thanks for working on it!

Unfortunately our CI is having issues atm so it's difficult to know with confidence nothing is broken. Hopefully this'll be resolved soon.

I'm pleased to see additional tests added, hopefully they cover most edge-cases?

I haven't got time to fully digest this right now, but I've left some initial thoughts below.

[MattSturgeon]

Having to define this explicitly feels fragile... What would happen if we add another plugin module but don't notice we need to define addition paths to link?

[stasjok]

What would happen if we add another plugin module but don't notice we need to define addition paths to link?

Additional paths will be missing from user runtime if combinePlugins is enabled, so it depends on what files are missing. For telescope here, it pretty much doesn't matter, because planets is a picker that is only for testing. Most plugins doesn't need any paths except standard.

I think in a description of this options should be clear, that this option is experimental, doesn't give any guaranties etc. So if it works for user's config, he'll get a performance boost, if not — sorry. I've added EXPERIMENTAL to the description for that purpose. But English is not native for me, if you have better description, we can update it.

[traxys]

I think the description is good. I think we can support this feature with less guarantees on breaking changes, i.e we won't add tests for all the plugins trying to enable the option, as this might be a too large maintenance burden, but we'll gladly include workarounds if people suggest them

[stasjok]

Unfortunately our CI is having issues atm so it's difficult to know with confidence nothing is broken. Hopefully this'll be resolved soon.

In a sibling PR all test are passed for two out of four arches. Those PR is branched on top of this, so I think all tests are passing. But to be honest I've noticed that most of the tests doesn't actually test functionality, only that config generated is valid and setup function is successful. In my tests I tried to test that my functionality actually work. Link to the x86_64 tests from a sibling PR: https://buildbot.nix-community.org/#/builders/146/builds/227

[traxys]

@stasjok FYI we recently merged a PR that seemed to massively decrease the amount of RAM used by the tests

[stasjok]

@stasjok FYI we recently merged a PR that seemed to massively decrease the amount of RAM used by the tests

I've already rebased, but all tests are still Broken pipe.

[MattSturgeon]

I've already rebased, but all tests are still Broken pipe.

That's a separate issue that should only affect buildbot (our CI). See nix-community/buildbot-nix#227.

we recently merged a PR that seemed to massively decrease the amount of RAM used by the tests

This should make it easier to run tests locally. Local tests should be unaffected by the broken pipe issue.

[traxys]

I think the description is good. I think we can support this feature with less guarantees on breaking changes, i.e we won't add tests for all the plugins trying to enable the option, as this might be a too large maintenance burden, but we'll gladly include workarounds if people suggest them

[traxys]

Thank you very much for this feature, and especially thank you for the in-depth tests, they make me much more confident that we can maintain this feature without it breaking silently!

[traxys]

Could you rebase on main, we should have fixed our CI problems!

[traxys]

I hate this error:

error: build of '/nix/store/3z9mf896pkksrhwkpbmm6sx8pcpznj4p-plugins-languages-treesitter-treesitter.drv' on 'ssh-ng://[email protected]' failed: builder for '/nix/store/3z9mf896pkksrhwkpbmm6sx8pcpznj4p-plugins-languages-treesitter-treesitter.drv' failed with exit code 65;
last 1 log lines:
> sandbox-exec: pattern serialization length 75852 exceeds maximum (65535)

We are going to need to disable this test on darwin
(See NixOS/nix#4119)

[traxys]

2. Optimize runtimepath further (basically set rtp to essential directories, removing everything else, mostly all system dirs). On my system with home-manager I have 14 useless items there. It's not yet implemented, but should be easy. For now I still have this in my config: https://github.com/stasjok/dotfiles/blob/36037f523185ba1409dd953999fda0f0db0dbd4f/nvim/init.lua#L1-L17, so I'm not in a hurry. It would depend on a feedback on this PR.

This would a nice feature, while it does not have many performance benefits, I find annoying having so many useless entries!

[traxys]

@Mergifyio queue

[mergify]

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at 0ac10f6

[traxys]

It seemed to work, can you squash the test in the tree-sitter implementation?

[stasjok]

2. Optimize runtimepath further (basically set rtp to essential directories, removing everything else, mostly all system dirs). On my system with home-manager I have 14 useless items there. It's not yet implemented, but should be easy. For now I still have this in my config: https://github.com/stasjok/dotfiles/blob/36037f523185ba1409dd953999fda0f0db0dbd4f/nvim/init.lua#L1-L17, so I'm not in a hurry. It would depend on a feedback on this PR.

This would a nice feature, while it does not have many performance benefits, I find annoying having so many useless entries!

I've implemented it here: https://github.com/stasjok/nixvim/commits/optimize-rtp/ (last two commits), but I don't like it much, that's why I haven't submitted a PR. It turns out somewhat awkward having this in the performance namespace with the options to add extra paths to rtp:

https://github.com/stasjok/nixvim/blob/2551d1afa61c5271b08228fbf43cd6a870f7df97/modules/performance.nix#L7-L22

In the codebase I need to check wrapRc and if performance.optimizeRuntimePath is enabled. So it kind of strange to allow adding additional paths to rtp, but it works only if optimization is enabled and doesn't work otherwise...

So my thoughts are, if there is an interest in this feature, it shouldn't be performance options. Nixvim should do it by default with additional options to allow adding extra paths to rtp and packpath. It gives several benefits:

1. nixvim will be pure by default (only paths managed by nixvim will be in rtp by default), but user always can add additional paths with options. Some plugins, like treesitter, can add the paths there automatically when nix parser management is disabled for example.
2. filesPlugin should never be added as a plugin. This is wrong to begin with. That's because user files should always be first in rtp. When filesPlugins is in the mix with other plugins, overriding plugin files may not work (depending on sorting order).
3. I think that there should be a default internal variable with a path to user files. By default it point to filesPlugin, but other modules (wrappers) can override it with it's own path (home-manager should set it to xdg config path that is perfectly known to it without using raw lua, like I did now, nixos can override it with etc directory with configs and so on). Final rtp is: configpath,plugins,nvimruntime,configafter.
4. Potentially we can manage rtp partially with wrapper args, in that case we can even drop wrapRc completely and just place init.lua to filesPlugin. But this is debatable, maybe managing rtp paths as the first lines in init.lua is enough plus allow users to add paths with raw lua. Or it can mixed approach: user config path is added with command line args (so that the directory with init.lua is known to neovim), other paths are managed in init.lua.

In the end since all above as pretty large refactor and it should also change how things are managed right now, I put this task aside and decided to just change my lua code with a simple nix config in my config (in home-manager options I can get a static paths to my config files, to plugin path and to Nvim runtime, so no lua loops and matching needed, just plain vim.o.runtimepath assignment.

[traxys]

Yeah I agree that it's something that we should do by default, we already do some modifications of the runtime path, so it wouldn't be too out of place

[traxys]

@stasjok thank you very much for that feature, it halved my startup time! From around 230ms to 100ms

[stasjok]

@stasjok thank you very much for that feature, it halved my startup time! From around 230ms to 100ms

Glad it worked for you!