Commit
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -350,6 +350,46 @@ this confusing. It will be included if | |
| :type '(repeat (choice symbol sexp)) | ||
| :group 'evil-collection) | ||
|
|
||
| (defcustom evil-collection-config | ||
| `((buff-menu :defer t) | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
jojojames
Author
Collaborator
|
||
| (calc :defer t) | ||
| (comint :defer t) | ||
| (debug :defer t) | ||
| (diff-mode :defer t) | ||
| (dired :defer t) | ||
| (edebug :defer t) | ||
| (eldoc :defer t) | ||
| (elisp-slime-nav :defer t) | ||
| (help :defer t) | ||
| (image :defer t) | ||
| (indent :defer t) | ||
| (dired :defer t) | ||
| (info :defer t) | ||
| (replace :defer t) | ||
| (occur :defer t) | ||
| (outline :defer t) | ||
| (package :defer t) | ||
| (package-menu :defer t) | ||
| (process-menu :defer t) | ||
| (simple :defer t) | ||
| (tab-bar :defer t) | ||
| (tabulated-list :defer t) | ||
| (xref :defer t)) | ||
| "The list of modes with special configuration. | ||
| These modes should match entries within `evil-collection-mode-list'. | ||
| This variable is consumed only by `evil-collection-setup'. | ||
| NOTE: The API of this variable may change drastically. | ||
| Currently supported keys: | ||
| :defer t or TIME in seconds to defer loading mode." | ||
| :type '(repeat (choice symbol sexp)) | ||
| :group 'evil-collection) | ||
|
|
||
| (defcustom evil-collection-key-whitelist '() | ||
| "List of keys that may be used by Evil Collection. | ||
| This is a list of strings that are suitable for input to | ||
|
|
@@ -842,6 +882,7 @@ instead of the modes in `evil-collection-mode-list'." | |
| reqs (cdr mode))) | ||
| (dolist (req reqs) | ||
| (with-eval-after-load req | ||
| ;; (message (format "Loaded %S..." req)) | ||
| (evil-collection-require m) | ||
| (funcall (intern (concat "evil-collection-" (symbol-name m) | ||
| "-setup"))) | ||
|
|
@@ -857,6 +898,58 @@ instead of the modes in `evil-collection-mode-list'." | |
| (evil-collection-require 'unimpaired) | ||
| (evil-collection-unimpaired-setup))) | ||
|
|
||
|
|
||
|
|
||
| (defun evil-collection-setup (&optional modes) | ||
| "Register the Evil bindings for all modes in `evil-collection-mode-list'. | ||
| ----------------------EXPERIMENTAL------------------------------------------ | ||
| This is a special wrapper over `evil-collection-init' that respects | ||
| configuration from `evil-collection-config'. This function is experimental, | ||
| so don't use if you don't breakages or API changes. | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. |
||
| If MODES is specified (as either one mode or a list of modes), use those modes | ||
| instead of the modes in `evil-collection-mode-list'. | ||
| ----------------------EXPERIMENTAL------------------------------------------" | ||
| (if modes | ||
| (or (listp modes) (setq modes (list modes))) | ||
| (setq modes evil-collection-mode-list)) | ||
| (let ((configs evil-collection-config) | ||
| (deferred-modes) | ||
| (delays)) | ||
| (dolist (config configs) | ||
| (let ((defer (plist-get (cdr config) :defer))) | ||
| (when defer | ||
| (push (car config) deferred-modes) | ||
| (push defer delays)))) | ||
| (let ((filtered-modes | ||
| (cl-remove-if | ||
| (lambda (mode) | ||
| (let ((filterp nil) | ||
| (modes (if (consp mode) mode (list mode)))) | ||
| (dolist (m modes) | ||
| (let ((x (memq m deferred-modes))) | ||
| (when x | ||
| (setf filterp t) | ||
| ;; `evil-collection-config' format is slightly different | ||
| ;; than `evil-collection-mode-list', so use the mode | ||
| ;; entry from the mode list instead. | ||
| (setf (car x) mode)))) | ||
| filterp)) | ||
| modes))) | ||
| (evil-collection-init filtered-modes)) | ||
| (message (format "Deferring: %S" deferred-modes)) | ||
| (dotimes (i (length deferred-modes)) | ||
| (let ((mode (nth i deferred-modes)) | ||
| (delay (nth i delays))) | ||
| ;; (message (format "Delaying %S..." | ||
| ;; (if (consp mode) (car mode) mode))) | ||
| (run-with-idle-timer | ||
| (if (numberp delay) delay 3) nil | ||
| (apply-partially 'evil-collection-init (list mode))))))) | ||
|
|
||
| (defvar evil-collection-delete-operators '(evil-delete | ||
| evil-cp-delete | ||
| evil-sp-delete | ||
|
|
||
11 comments
on commit d97e6d5
There was a problem hiding this comment.
@condy0919 @noctuid Would appreciate a look, code is kind of messy but it seems to work OK for me. Got the package loading down substantially:
;; BEFORE
;; Configuring package evil-collection...done (0.105s)
;; Loading package evil-collection...done (0.109s)
;; AFTER
;; Configuring package evil-collection...done (0.018s)
;; Loading package evil-collection...done (0.022s)
Would be nice to support more keywords in this list and maybe even key themes that can be automagically configured for specific packages.
Anyways, I consider this feature experimental at the moment, I don't expect the calling semantics/implementation/API to be set in stone so appreciate any feedback. Otherwise I'll continue dogfooding it on my own.
There was a problem hiding this comment.
Is the point just to decrease load time? The way I use evil-collection is very piecemeal, only using evil-collection-init for single package at a time after the corresponding package has loaded, so I won't test this myself.
If there will be new keywords added, I think an alist (or plist) of mode to a plist of keyword arguments is fine. It also makes sense for :delay. Alternatively, it might make sense to just have a separate variable like evil-collection-defer-modes instead of specifying :defer t for all of them.
That said, I think it would be better to just provide an alternative to evil-collection-init that does the suggestion in evil-collection-init and uses eval-after-load automatically to defer requiring other evil-collection files until they are actually needed without any user configuration required. Unless I am missing some downside or problem.
There was a problem hiding this comment.
Is the point just to decrease load time?
This change is primarily for that but I'm interested in supporting other configuration points in the same list. I'm finding too many potential defcustoms littering the various modes, so want one place to configure them all.
Also yeah, I want evil-collection not to contribute to the overall slow start emacs generally gets a bad rap about so getting it from 100+ ms to under 20ms is pretty nice IMO while still technically loading everything (aka no additional configuration from the user).
for single package at a time after the corresponding package has loaded, so I won't test this myself.
Sounds good, I am dogfooding it myself so hopefully functionally it is reasonable.
and uses eval-after-load automatically to defer requiring other evil-collection files until they are actually needed without any user configuration required. Unless I am missing some downside or problem.
I'm probably missing the suggestion somewhere but I'd assume we need to use an idle-timer since a lot of these modes are 'needed' immediately at startup. (e.g. would trigger with-eval-after-load right away)
There was a problem hiding this comment.
I'm finding too many potential defcustoms littering the various modes, so want one place to configure them all.
I think that makes sense.
I'm probably missing the suggestion somewhere but I'd assume we need to use an idle-timer since a lot of these modes are 'needed' immediately at startup. (e.g. would trigger with-eval-after-load right away)
I forgot that evil-collection-init already does this, but in the new evil-collection-config list, it looks like there are only 7 features that are loaded immediately with emacs -Q. Some more of them will be loaded by packages commonly required at startup (e.g. info by straight.el, calc by evil). The evil-collection files for the others already shouldn't be loaded during startup when using evil-collection-init. For me, they are loaded only after startup: e.g. debug by edebug, edebug by helpful, comint by org, diff-mode by magit, xref by lispy, not sure about outline. Dired is loaded whenever I use dired. I don't have elisp-slime-nav installed. Occur, package, package-menu, and process-menu are not loaded at all in my current Emacs.
For packages like dired and especially external packages like elisp-slime-nav, I think the responsibility should be on the user to not unnecessarily load them in their init files if they want faster startup.
Here are the features that are loaded for me with emacs -Q (along with calc and info):
- eldoc, tab-bar, simple, tabulated-list - very small evil-collection configuration
- help, image, info, calc - bigger
- replace - not sure, does it have a dedicated file?
I'm not sure how much of a percentage of your evil-collection startup time is from these vs. the others, but even though eval-after-load doesn't delay them, "transient" hooks or advice could. By "transient" I mean a hook or advice that runs once and then removes itself so it only runs once (eval-after-load does this in some cases). For example, I'm not sure if evil-set-intial-state would work in help-mode-hook (haven't tested), but I think making the keybindings should. Similarly, calc keybindings are not actually necessary until you use calc interactively.
Instead of optionally configuring certain packages to defer further after eval-after-load using idle timers, I think it would be better to optionally defer them until just before they are needed.
while still technically loading everything (aka no additional configuration from the user).
I'd assume we need to use an idle-timer
I personally avoid using idle timers wherever possible for package loading. The problem is that if you have a bunch of packages that all are delayed the same amount of time (e.g. 3s idle time for evil-collection modes and then the user also has a lot of :defer 3s), then you can get an unnecessarily big freeze. With a basic delay implementation, manual configuration of the delay times is required to prevent this issue. This is why Doom, for example, uses more specific strategies where possible (e.g. eval-after-load, hooks, and advice) and has :defer-incrementally (if you're familiar with it) to space out loading packages in idle time automatically.
Even if a significant amount of your evil-collection startup is from packages that are necessarily loaded immediately, I think it may be better to prefer correct behavior and load the evil-collection files immediately. The other problem with idle timers is that they generally activate too early or too late. Even though it may be rare, delaying loading evil-collection-help using an idle timer could cause issues if the user immediately opened a help buffer.
The more complicated solution would be to let the user handle further optimization if they wanted to or to use hooks (simpler) or advice (if necessary) instead of delays to ensure that evil-collection-help, evil-collection-calc, evil-collection-info, etc. are loaded just in time. Whether the extra complexity is worth it is up to you, but I don't think it would be too bad.
the overall slow start emacs generally gets a bad rap about
On a related note, I'm currently working on a suite of packages meant to simplify init.el configuration and reduce Emacs startup time. For example, once.el is intended to drastically simplify the process of intelligently deferring packages and package configuration. If that's of interest to you, any feedback would be appreciated (the documentation is there, but I would not try to actually use it at this stage because it is not ready/may be broken).
There was a problem hiding this comment.
For me, they are loaded only after startup: e.g. debug by edebug, edebug by
helpful, comint by org, diff-mode by magit, xref by lispy, not sure about
outline. Dired is loaded whenever I use dired. I don't have elisp-slime-nav
installed. Occur, package, package-menu, and process-menu are not loaded at
all in my current Emacs.
I have no idea honestly. It's really timeconsuming troubleshooting which package
is loading what. For myself, thsoe packages load but I also have a ton of
packages that are kind of "required" at the start. The list there is just the
list I got from printing out which packages evil-collection instantly prepares.
For packages like dired and especially external packages like elisp-slime-nav,
I think the responsibility should be on the user to not unnecessarily load
them in their init files if they want faster startup.
I agree somewhat, but letting them defer it here is reasonable too, afterall
we're letting them defer our setup and not the package load.
I think it would be better to optionally defer them until just before they are needed.
If you mean like other types of defering that's unrelated to timers, I agree but
just quickly implemented for :defer since that seems to be the simplest to
implement.
I personally avoid using idle timers wherever possible for package
loading. The problem is that if you have a bunch of packages that all are
delayed the same amount of time (e.g. 3s idle time for evil-collection modes
and then the user also has a lot of :defer 3s), then you can get an
unnecessarily big freeze. With a basic delay implementation, manual
configuration of the delay times is required to prevent this issue. This is
why Doom, for example, uses more specific strategies where possible
(e.g. eval-after-load, hooks, and advice) and has :defer-incrementally (if
you're familiar with it) to space out loading packages in idle time
automatically.
Totally agree that more/better/smarter package deferring would be good. I'm
hitting the first use case of just allowing deferring in the first place and
having it reduce down to .02 is reassurring with just idle-timer code.
Also the effect of the idle timers here should only be between 80ms (afterall,
that is the time saved) so the user would need a lot more idle timers to freeze
their emacs.
I think it may be better to prefer correct behavior and load the
evil-collection files immediately. The other problem with idle timers is that
they generally activate too early or too late. Even though it may be rare,
delaying loading evil-collection-help using an idle timer could cause issues
if the user immediately opened a help buffer.
Yup, that's the problem with idle timers in general and also in use-package's
:defer. I think that's ok and a reasonable tradeoff though. If we had another
versions of deferral, it'd be even better. After all, I use
:defer,:commands,:bind,:hook etc etc depending on what I want.
The more complicated solution would be to let the user handle further
optimization if they wanted to or to use hooks (simpler) or advice (if
necessary) instead of delays to ensure that evil-collection-help,
evil-collection-calc, evil-collection-info, etc. are loaded just in
time. Whether the extra complexity is worth it is up to you, but I don't think
it would be too bad.
I'd like to support additional defer-based code like what is seen in use-package
but happy with :defer for now. Might look into :hook later.
On a related note, I'm currently working on a suite of packages meant to
simplify init.el configuration and reduce Emacs startup time. For example,
once.el is intended to drastically simplify the process of intelligently
deferring packages and package configuration. If that's of interest to you,
any feedback would be appreciated (the documentation is there, but I would not
try to actually use it at this stage because it is not ready/may be broken).
Looks cool, will look into.
One note is to add the performance penalties of loading this package. Sometimes
I get scared off into thinking a package I'm adding is -also- contributing to
the wildfire.
There was a problem hiding this comment.
It's really timeconsuming troubleshooting which package is loading what.
I have some code in my configuration, so I can do emacs --record-requires to check what requires what during initialization or emacs --record-requires --keep-benchmarking to record requires forever. That's how I tested for those features.
I agree somewhat, but letting them defer it here is reasonable too, afterall we're letting them defer our setup and not the package load.
Agreed, though I'm not sure it makes sense to do by default for all packages. It makes sense to handle calc by default since evil requires it, but I'm not sure dired ever needs to be required before you would actually use it.
Also the effect of the idle timers here should only be between 80ms
It's only really a concern if the user combines it with a bunch of their own :defer 3s, but I think the bigger issue is it potentially loading too late.
I think using transient hooks would be better (in cases they work correctly), but it is probably overkill at this point.
One note is to add the performance penalties of loading this package. Sometimes I get scared off into thinking a package I'm adding is -also- contributing to the wildfire.
It's something I've considered for the new packages I'm working on. Some of them will be entirely macros and will therefore have no penalty when used in a compiled init file (like use-package). The rest will be split into small files so that you can load only what you need. In the case of once.el, it's currently <500 LOC (remove the docstrings and it's a lot less). The fact is that if you want more complicated ways to defer packages (like Doom provides with :defer-incrementally, :after-call, defer-until!, etc.), you need new code Emacs does not provide by default. Larger packages like evil, org, dired, etc. are the biggest issues.
One thing that imight be nice would be to allow :defer to take a function. This could allow someone to opt-in to use some more complex deferral tool instead of idle timers:
;; e.g. add this to the list
(list 'help :defer (apply-partially #'once-x-call 'help-mode-hook))
;; evil-collection would do this if the :defer value is a function
(funcall defer-function setup-function)
There was a problem hiding this comment.
I have some code in my configuration, so I can do emacs --record-requires to check what requires what during initialization or emacs --record-requires --keep-benchmarking to record requires forever. That's how I tested for those features.
Definitely share :)
Agreed, though I'm not sure it makes sense to do by default for all packages. It makes sense to handle calc by default since evil requires it, but I'm not sure dired ever needs to be required before you would actually use it.
I'm selfishly just putting whatever is blocking my own startup in there. If users want to use it, I'm pretty sure they would just do (setq config '(there only list here)) or just accept whatever I have in mine.
One thing that imight be nice would be to allow :defer to take a function. This could allow someone to opt-in to use some more complex deferral tool instead of idle timers:
Might not be the worst thing in the world to integrate your once library if the performance/startup penalty is near-zero, so we can have better deferral functions for free.
I think a function seems reasonable though, I'll have to look into adding it.
There was a problem hiding this comment.
Definitely share :)
See here. Probably this could be improved. There are a few cases where it doesn't work (load-file-name is not set), which is why I don't know what loaded outline for me.
Might not be the worst thing in the world to integrate your once library if the performance/startup penalty is near-zero, so we can have better deferral functions for free.
Assuming you're open to that, after it's on MELPA, I can check for these packages that it will actually work correctly to, for example, run evil-collection-help-setup in help-mode-hook. Assuming it does, I can make a PR to configure the packages that make the most sense to defer to use hooks or whatever instead. We can benchmark to see if loading once.el is actually an issue, but the part that evil-collection would need to require will probably be <=400 lines of actual code. If it's still too much, I could alternatively make smaller PR with only the necessary code extracted.
Once.el (or something else) could also potentially replace evil-delay in evil-collection. I need to do some more investigation into why, but using evil-delay to delay keybindings until a keymap has loaded can in some cases massively slow down Emacs initialization (see this longstanding general.el issue). Evil-collection may not have this issue, but I can check if it does.
One other minor benefit of of using once would be that you could also, if you wanted to, replace with-eval-after-load calls with the once.el equivalent which will not unnecessarily add to after-load-alist if the package is already loaded. If a feature is already loaded, it will just run the code and do nothing else (though this would probably make an insignificant difference in startup speed).
There was a problem hiding this comment.
Assuming you're open to that, after it's on MELPA, I can check for these packages that it will actually work correctly to, for example, run evil-collection-help-setup in help-mode-hook. Assuming it does, I can make a PR to configure the packages that make the most sense to defer to use hooks or whatever instead. We can benchmark to see if loading once.el is actually an issue, but the part that evil-collection would need to require will probably be <=400 lines of actual code. If it's still too much, I could alternatively make smaller PR with only the necessary code extracted.
Yeah, definitely open to it, considering this area will be experimental territory for the time being.
Ignoring the below general bug, looking back at the last few years of work on evil-collection, I might've preferred pulling in general for a better API than the current evil-collection-define-key. Mainly the fact that "evil-collection-define-key" is too long to type and kind of ugly. In my own init files away from evil-collection, I use evil-define-key instead. A more consistent experience using one mapper would've been better IMO so in this case, I'd prefer to lean on other libraries instead of writing my own stuff if I can. Like mentioned, if the performance/latency from including it is negligible, I'm open to it.
There was a problem hiding this comment.
As an aside, even general-def is pretty long for my taste but still much better than evil-collection-define-key. I would've preferred something like vim-bind-key/vi-bind/vi-def/vi-bind-key etc etc than any of the current namespaces.
There's the https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=58055b5fc330689234cafb51398844f6e5791077 shorthands feature that came out recently? But I'm assuming that's not backwards compatible and won't be usable in packages until we hit like emacs 35.
There was a problem hiding this comment.
Yeah, definitely open to it, considering this area will be experimental territory for the time being.
Okay, I will look into it.
Ignoring the below general bug, looking back at the last few years of work on evil-collection, I might've preferred pulling in general for a better API than the current evil-collection-define-key. Mainly the fact that "evil-collection-define-key" is too long to type and kind of ugly. In my own init files away from evil-collection, I use evil-define-key instead. A more consistent experience using one mapper would've been better IMO so in this case, I'd prefer to lean on other libraries instead of writing my own stuff if I can. Like mentioned, if the performance/latency from including it is negligible, I'm open to it.
General is really not suited towards being used in other packages for the reasons I've mentioned before (e.g. it includes a bunch of extra stuff in addition to the definer; once.el originally started in general). The general.el rewrite I am currently working on, however, will be much more focused/modular and have no overhead at all: provided that the package using it is byte compiled (which every major package manager does), the general.el rewrite does not need to be loaded during initialization. This means it would be far more suitable for use in some library like evil-collection. Someone has suggested it might even make sense to have in core Emacs. That would make it a no-brainer for use in libraries, but I think it's probably too opinionated to be builtin.
I think evil-collection is stuck with evil-collection-define-key, but maybe this will be useful for other libraries. evil-collection-define-key could use a different key definer library under the hood, but a wrapper is still needed to support the evil-collection-specific functionality like evil-collection-key-whitelist.
Like mentioned, if the performance/latency from including it is negligible, I'm open to it.
For reference, annalist.el is >2x as big as once.el and is required by evil-collection already. I think we can defer requiring annalist and doing any recording until the user actually runs evil-collection-describe-bindings to further speed up evil-collection initialization. Or, to reduce code duplication, the evil-collection-define-key implementation could be switched to use my general.el rewrite (when it is eventually ready) which will already support this.
general-def is pretty long for my taste
If it's your own config, you can always alias it or evil-define-key to def. And if you're using use-package or an equivalent, you can pick whatever keyword name you want. The length doesn't matter that much to me personally. Evil support for my new general-like key definer will be implemented in imp.el, so one could at least get a imp-def or just imp (doesn't get shorter than that). I don't think there is any "vim" package, so you could maybe get away with make one just for the namespace.
There's the https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=58055b5fc330689234cafb51398844f6e5791077 shorthands feature that came out recently? But I'm assuming that's not backwards compatible and won't be usable in packages until we hit like emacs 35.
I hadn't seen that before now. Still not much of anything compared to CL's package system. I think I'd still use an alias instead for definers, but that would potentially be nice for some utility library that provides a bunch of functionality (like an "import x as ...").
Looks like an unnecessary backquote.