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

Big breaking change: Make the compiled file do less and implement autorecompilation #402

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wbthomason
Copy link
Owner

@shadmansaleh @akinsho Now that #331 is merged, this PR will be the big breaking change to:

  1. Require users to load their packer config always
  2. Reduce the compiled file to lazy-loaders and any precomputed paths we can generate/luarocks setup
  3. Change to use the "live" values from the spec for setup and config as well as cond and after
  4. Implement autorecompilation by storing hashes of the strings that specify lazy-load conditions and regenerating the compiled file if the current values don't match
  5. Move the default compiled file location to stdpath('cache')
  6. Clean up the code while we're in there, try to use luv instead of e.g. vim.fn wherever possible, in general simplify and tidy things which have grown cruft over the course of packer's existence so far.

@wbthomason
Copy link
Owner Author

Oh: it's probably out of scope for this PR, but I want to design this change such that it enables adding plugins imperatively, i.e. via a command. Probably a good idea to look at how paq added this, specifically w.r.t. how they add the specs for the imperatively added plugins to the overall set of plugin specifications.

@wbthomason
Copy link
Owner Author

This PR is also going to need some mechanism for migrating users, e.g. by deleting their old packer_compiled files if they exist. So I guess that's (7),

@shadmansaleh
Copy link
Contributor

shadmansaleh commented Jun 13, 2021

Implement autorecompilation by storing hashes of the strings that specify lazy-load conditions and regenerating the compiled file if the current values don't match

Will auto compilation happen at startup ? when will the check happen actually?

Move the default compiled file location to stdpath('cache')

Any comment on #307 . Things like #393 will continue to be an issue but more unexpectedly as some plugins will work and some won't based on what filesname packer choses for the loader.

Require users to load their packer config always

What happens when they are out of sync? Say I wanted to test out someone elses config or a framework more importently trying to isolate a plugin to file a bug report .

If I comment out some plugins will that be an erroe?

@wbthomason
Copy link
Owner Author

Will auto compilation happen at startup ? when will the check happen actually?

Right now, I'm thinking the check happens at startup; recompilation happens on packer operations or at startup iff a difference is detected.

Any comment on #307 . Things like #393 will continue to be an issue but more unexpectedly as some plugins will work and some won't based on what filesname packer choses for the loader.

#307 is on my queue to reply to. From what I remember the last time I looked at it, it's somewhat orthogonal to this set of changes (i.e. we can make the move to be fully-opt if we decide to without causing a breaking change), but, regardless, the name of the compiled file should not matter - it won't be somewhere that Neovim sources, but will be explicitly loaded/run by packer during startup.

What happens when they are out of sync? Say I wanted to test out someone elses config or a framework more importently trying to isolate a plugin to file a bug report .

The idea of autocompilation would be that, when you're testing another config, your compiled file automatically gets rebuilt and sourced, and when you go back to your usual config, it gets rebuilt and sourced again for you. So all you see is a slightly slower startup time those two times. I think there is a question here around how we handle using different configs e.g. with cleaning "unused" plugins, etc., but in my mind that's more of an argument in favor of #307.

If I comment out some plugins will that be an erroe?

No. Without #307, if they're start, they'll still get loaded, and regardless if they have lazy-loaders those will get removed. This shouldn't be an error, though.

@shadmansaleh
Copy link
Contributor

Ok got it .

regardless, the name of the compiled file should not matter - it won't be somewhere that Neovim sources, but will be explicitly loaded/run by packer during startup.

I wasn't meaning compiled file but the file in packer.nvim/plugin that loads that compiled file.

@shadmansaleh
Copy link
Contributor

So we need to make use handle creation of _G.packer_plugins. The compiled file will just invoke load module and load module executes based config/setup/cond from _G.packer_plugins . Does it seem right ?

@wbthomason
Copy link
Owner Author

I wasn't meaning compiled file but the file in packer.nvim/plugin that loads that compiled file.

Ah, my bad. Yes, I think that without #307 or similar, which file the user chooses to call packer's loader from could cause issues similar to #393.

So we need to make use handle creation of _G.packer_plugins. The compiled file will just invoke load module and load module executes based config/setup/cond from _G.packer_plugins . Does it seem right ?

I have a slightly different architecture in mind:

  • use still just adds plugins to the managed set
  • "Automatically" at the end of packer.startup or manually if the user doesn't want to use startup, a function is called which:
    1. Loads the compiled file, which checks if it's out of date and rebuilds and resources itself if so, then sets up Luarocks paths, lazy-load triggers, precomputed path things, and maybe other things it helps to precompute in the future.
    2. Invokes the load module for all the start functions, handles sequenced loading, and handles conditional loading
  • The load module does basically what you say, using the config/setup values from _G.packer_plugins.

@wbthomason wbthomason marked this pull request as draft June 13, 2021 19:42
@akinsho
Copy link
Collaborator

akinsho commented Jun 13, 2021

@wbthomason this all sounds good.

One question, would a user still have to keep all they configs/setup in the main packer file. Not sure anything that's been mentioned implies that but we previously discussed that maybe being required and I'm curious if that might be a new restriction but I don't see why.

I'm wondering how you want to tackle this I appreciate that there's a breaking change and maybe the sense that this can/should be done in one PR but I'm wondering if this work can't be chunked up into bits maybe some code changes that aren't used till a particular point where things finally get switched over.

I'd be happy to help with bits of it as time permits but I guess I'm wondering how we can split this out in a way where it's easier for multiple people to pick up chunks until we reach a stage where we want to flip the switch and move everything over. Or maybe making incremental PRs into this one 🤷🏾 .

@wbthomason
Copy link
Owner Author

would a user still have to keep all they configs/setup in the main packer file

They should still be able to split their config across files, but must ensure that any files specifying plugins they want to use are sourced before they call the new loader function in their config. This is compatible with all the current use cases I'm aware of; they could always just call the loader function in a file in ~/.config/nvim/plugin/ or something similar.

how we can split this out in a way where it's easier for multiple people to pick up chunks

Yeah, splitting this up would be smart. I think I prefer to make multiple PRs into this PR branch, but I'm open to suggestions.

I think there are a few separable chunks to this work:

  1. Implement the new loader function (including pulling functionality out of the compile module)
  2. Implement the autorecompilation mechanism
  3. Implement whatever migration logic we need (i.e. printing warnings, deleting old compiled files)
  4. (after the rest) cleanup/simplification work

An implementation of the ideas in #307 could also fit into this big set of changes, as part of (1) and (2). That said, I'm not sure if it's wise to change so much all at once.

@clason
Copy link

clason commented Jun 14, 2021

To that last point, I currently (with the new Lua runtime PR merged) have

  1. a plugin/packer.lua that just contains the list of plugins and a require'packer'.startup {plugins, config = config} call (no plugin config!)
  2. for every plugin, a plugin/<plugin>.lua file with the corresponding setup call etc.
  3. put the packer_compiled.vim into stdpath('data')/site/pack/loader/start/packer.nvim/plugin/

This is working very well for me right now, and I would like to keep it this way ;)

@wbthomason
Copy link
Owner Author

wbthomason commented Jun 14, 2021

@clason That should still be 100% compatible with both this PR and #307.

EDIT: Actually, I spoke a bit too quickly. That use case is definitely fine under this PR. It would be broken by #307, I think, because it's possible that some of your plugin/<some actual plugin>.lua files get sourced before your plugin/<packer stuff without config>.lua file. That's OK now because you presumably have everything as start, but under #307 would be a problem because the plugins wouldn't be loaded until your packer file with the startup call is sourced. The fix for this would be ensuring that this file gets loaded in your init.lua. Would that be an acceptable amount of breakage?

@shadmansaleh
Copy link
Contributor

shadmansaleh commented Oct 19, 2021

I think I'll propose removing compilation again . I've been testing in my branch without compilation . I'm getting ~10% startup time degradation ( couple of ms :P ) without compilation though I think most of it is from calling manage_all_plugins at startup . Also that implementation isn't optimal ( I basically just short circuited the compile module to run on startup ) . I feel like compilation is more trouble then the benefits it offers . This way upvalues directly work too

@wbthomason
Copy link
Owner Author

Removing manual compilation (though I still plan to "compile" - more like cache to file - some information that can be automatically updated and saves us from needing to do some expensive FS operations at startup) is still a priority. I'm disentangling that from #307 for now. The main blocker right now is just that my (admittedly limited) maintenance time for packer has gone to dealing with issues and smaller bugs, so I simply haven't had time to get to this.

This is the work I have planned to move away from manual compilation:

Code changes

  1. Restructure the main packer module to be minimal cost to require. It should define init, startup, use (which will still be a stub that just adds specs to a table), a new finish (or some better name), and the auxiliary functions used by startup (e.g. command creation, the end-of-operation hooks), and expose (but not define/require) the management operations.
    1. init should create the managed plugin set and merge user configuration with defaults
    2. use should add plugin specs to the managed set without processing them
    3. startup should call init, run the user's specification function (or process their table of specs, etc.), call finish, and then create commands and run a post-startup hook.
    4. finish will call a redone version of manage_all_plugins and a new create_loaders function, described below, as well as sourcing a file with cached information on e.g. ftdetect files that need to be sourced for opt plugins/other information we can compute at update/install time.
  2. Make managing plugins faster. Managing plugins now has two forms:
    1. At startup, we only care about getting the information we need to make lazy-loaders. This means checking for specific keys in specs, but ignoring things like setting up plugin updaters, etc.
    2. At management operation time (i.e. installs, updates), we need the rest of the information, which can be added into the partially processed plugins from startup.
  3. create_loaders does what compile currently does, with some changes to keep the profiling feature and account for doing things at runtime rather than compiling to output (i.e. no more string.dump annoyances)
  4. Refactor the management operations to not live in the main packer module to avoid the associated startup cost
  5. Misc. other refactoring - nothing big (like the git module rewrite I want to do, or Idea: Install all plugins as opt plugin . #307, which I'm decoupling from this PR), but minor cleanup
  6. Add a shim to delete old packer_compiled files.

Docs/community changes

  1. Remove references to manual compilation
  2. Recommend that users call startup (or init/use/finish manually) in their init files. This is technically not necessary with this change, but sets the stage for possible later changes like Idea: Install all plugins as opt plugin . #307 and will reduce the amount of angst caused by lazy-loaders being created only when plugin/ is sourced.
  3. Make a "migration" document describing these changes
  4. Advertise the change (Reddit, Gitter) and get users to try the PR pre-merge
  5. Probably a bunch of things I've forgotten

If the community wants this prioritized over other bug fixes and issue maintenance, I'm happy to focus on it.

@shadmansaleh
Copy link
Contributor

shadmansaleh commented Oct 20, 2021

Ok if you want to do it that way . Actually I'm not much concerned about #307 ever since I figured I can change XDG env vars to move packers location. Also I can very well setup a system like #307 from currently available config options.

ftdetect files that need to be sourced for opt plugins/other information we can compute at update/install time.

I feel like we don't need to keep track of ftdetect files.
We can just run

runtime! OPT ftdetect/**/*.vim
runtime! OPT ftdetect/**/*.lua

Nvim itself basically runs runtime for sourcing rtp stuff don't think it'll be horribly slower . Also I think bfredl is working on some directory caching for source.

@wbthomason
Copy link
Owner Author

@shadmansaleh Ah, I didn't know about OPT as a valid specifier to runtime. That may indeed obviate caching those paths.

@weilbith
Copy link
Contributor

weilbith commented Dec 22, 2021

Hey,
sorry I'm a bit late on the party. I guess its probably a bit late for my input.

So, assuming users are still able to nicely maintain their plugin registrations, this is a comparison on how fast the currently compiled file gets sourced versus a user having to load all its plugin registrations. Assuming the load function is equally fast in both cases. Here I only care about start-up and that all plugins have their setup and config available.
For example does this mean for me that I need to source/parse ~50 files from my runtime (scales linear with number of plugins) extra on start-up (runtime! OPT plugin_registration.lua). Plus for me I also have an intermediate layer of how I register plugins (I do not directly call Packers use or startup function). So far this overhead is all fine for me as it only applies when I actively decide to update/compile my plugins. On every start-up this doesn't run, so it doesn't matter how much overhead it is. I truely love this separation. Though I see the advantages.

Today I was working on a better stringify function for the compilation (as string.dump only works for functions). It allows to also have metatables being put into the compilation output. This for example allows to create a config as a table that includes all the relevant context/arguments (must not be the name and the specification but can be anything) and attach a __call meta function to it. After all this is basically a closure implementation that can be stringified and parsed again. I use load instead of loadstring for this. It works quite smoothly and is very powerful.
But I guess after these changes it doesn't matter anymore.

@wbthomason wbthomason force-pushed the refactor/breaking-change-reduce-compilation-and-autorecompile branch from 37c58ae to 57dbc3d Compare February 26, 2022 23:47
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.

Make lazy-loader compilation more automatic
5 participants