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

Optimize general-define-key #180

Open
noctuid opened this issue Feb 27, 2020 · 14 comments
Open

Optimize general-define-key #180

noctuid opened this issue Feb 27, 2020 · 14 comments

Comments

@noctuid
Copy link
Owner

noctuid commented Feb 27, 2020

No work has been done to optimize general.el. Generally, this works out okay since most keybindings can be in a with-eval-after-load, and general will automatically delay them if the keymap does not exist. At some point, I should profile things (definitely will if I ever rewrite or split general). For now, minimizing the work done in general-define-key before the delay should help.

As @hlissner points out here, general can impact startup time significantly if using a lot prefix keywords. This could be done in general--define-key instead, so that it is deferred until the keymap(s) exist,. This would reduce the initial time general-define-key takes when it is possible to delay keybindings (and spreading out the total time when the user is autoloading packages).

@hlissner
If you remember, were most of these calls to general that were slowing things down making the keybinding immediately (making the prefix concatenation itself the issue) or were they defining keys for keymaps before the keymaps were available (making the fact that prefix concatenation is done before the delay the issue)?

@willbush
Copy link

willbush commented Apr 19, 2020

Here is a simple init.el files that reproduces the issue:

;;; init.el -*- lexical-binding: t; -*-

(require 'package)

(add-to-list 'package-archives
             '("melpa" . "https://melpa.org/packages/"))

(package-initialize)

;; bootstrap use-package with package.el
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))

;; ensure everything is installed
(setq use-package-always-ensure t)

(require 'use-package)

(use-package which-key :config (which-key-mode +1))

(use-package general)

;; Leader key bindings
(general-def
  :prefix "SPC"
  :non-normal-prefix "M-SPC"
  :states '(normal visual emacs)
  :keymaps 'override
  "TAB" 'mode-line-other-buffer
  "!" 'shell-command
  "'" 'my/open-shell
  "?" 'counsel-descbinds
  "F" '(:ignore t :wk "frame")
  "FD" 'delete-other-frames
  "Fd" 'delete-frame
  "Fi" 'iconify-frame
  "Fm" 'toggle-frame-maximized
  "Fn" 'make-frame
  "Fo" 'other-frame
  "S" '(:ignore t :wk "spell-checking")
  "SPC" 'counsel-M-x
  "Sb" 'flyspell-buffer
  "Sc" 'flyspell-correct-wrapper
  "Sn" 'flyspell-correct-next
  "Sp" 'flyspell-correct-previous
  "Sr" 'flyspell-region
  "a" '(:ignore t :wk "apps")
  "aD" 'deer-jump-other-window
  "aE" 'esup
  "ad" 'deer ;; minimal ranger dired
  "ae" '(:ignore t :wk "direnv")
  "aeU" 'direnv-update-directory-environment
  "aea" 'direnv-allow
  "aem" 'direnv-mode
  "aeu" 'direnv-update-environment
  "af" 'elfeed
  "ap" '(:ignore t :wk "profiler")
  "apk" 'profiler-stop
  "apr" 'profiler-report
  "aps" 'profiler-start
  "apw" 'profiler-report-write-profile
  "ar" 'ranger
  "as" 'speedbar
  "au" 'disk-usage
  "aw" 'wttrin
  "b" '(:ignore t :wk "buffer")
  "bD" 'my/kill-all-buffers
  "bH" 'my/kill-all-buffers-then-switch-to-dashboard
  "bI" 'ibuffer
  "bb" 'ivy-switch-buffer
  "bd" 'my/kill-this-buffer
  "bh" 'my/switch-to-dashboard
  "bi" 'counsel-ibuffer
  "bk" 'kill-buffer ;; requests buffer to kill
  "bm" 'my/switch-to-messages
  "br" 'my/revert-buffer-no-confirm
  "bs" 'my/switch-to-scratch
  "c" '(:ignore t :wk "comment")
  "cl" 'comment-line
  "cr" 'comment-or-uncomment-region
  "f" '(:ignore t :wk "file")
  "fd" 'fd-dired
  "ff" 'counsel-find-file
  "fs" 'save-buffer
  "fy" 'my/yank-and-show-buffer-full-path
  "fz" 'counsel-fzf
  "g" '(:ignore t :wk "go")
  "gf" 'find-function
  "gg" 'avy-goto-char-timer
  "gv" 'find-variable
  "h" '(:ignore t :wk "help")
  "hI" 'info-display-manual
  "hd" '(:ignore t :wk "describe")
  "hdB" 'evil-collection-describe-bindings
  "hdK" 'my/describe-keymap
  "hdb" 'counsel-descbinds
  "hdc" 'describe-char
  "hdf" 'counsel-describe-function
  "hdg" 'general-describe-keybindings
  "hdk" 'describe-key
  "hdm" 'describe-mode
  "hdp" 'describe-package
  "hds" 'counsel-info-lookup-symbol
  "hdt" 'describe-theme
  "hdu" 'counsel-unicode-char
  "hdv" 'counsel-describe-variable
  "hi" 'info
  "hl" 'counsel-find-library
  "hn"  'view-emacs-news
  "ht" 'evil-tutor-start
  "hw"  'woman
  "m" '(:ignore t :wk "magit")
  "mG" 'hydra-git-gutter/body
  "mI" 'magit-init
  "mc" 'magit-clone
  "md" 'magit-dispatch
  "mg" 'counsel-git-grep
  "mi" 'magit-gitignore-globally
  "ml" 'counsel-git-log
  "mm" 'magit-status
  "mt" 'git-timemachine
  "n" '(:ignore t :wk "narrow")
  "nf" 'narrow-to-defun
  "np" 'narrow-to-page
  "nr" 'narrow-to-region
  "nw" 'widen
  "o" '(:ignore t :wk "org")
  "oS" 'org-store-link
  "oa" 'org-agenda
  "oc" 'org-capture
  "oi" 'org-indent-mode
  "or" 'org-refile
  "os" 'org-save-all-org-buffers
  "ot" 'org-toggle-link-display
  "p" 'projectile-command-map
  "q" '(:ignore t :wk "quit")
  "qQ" 'save-buffers-kill-emacs
  "qq" 'save-buffers-kill-terminal
  "r" '(:ignore t :wk "rapid") ;; emphasize easy access over mnemonics
  "rl" 'counsel-load-theme
  "rs" 'save-buffer
  "s" '(:ignore t :wk "search")
  "sD" '(counsel-rg :wk "counsel-rg-directory")
  "sc" 'evil-ex-nohighlight ;; mnemonic is search clear
  "sd" 'deadgrep
  "so" 'counsel-outline
  "sr" 'counsel-rg
  "ss" 'swiper-multi
  "sz" 'counsel-fzf
  "t" '(:ignore t :wk "toggle")
  "tc" 'fci-mode
  "tf" 'auto-fill-mode
  "tg" 'my/toggle-golden-ratio
  "tl" 'toggle-truncate-lines
  "tm" 'counsel-major ;; switches major mode
  "tn" 'display-line-numbers-mode
  "ts" 'flyspell-mode
  "tt" 'display-time-mode
  "tv" 'my/toggle-adaptive-visual-fill-column
  "w" '(:ignore t :wk "window")
  "w-" 'split-window-vertically
  "w/" 'split-window-horizontally
  "wB" 'balance-windows-area
  "wE" 'evil-window-move-very-top
  "wI" 'evil-window-move-far-right
  "wM" 'evil-window-move-far-left
  "wN" 'evil-window-move-very-bottom
  "wb" 'balance-windows
  "wd" 'delete-window
  "we" 'evil-window-up
  "wg" 'golden-ratio
  "wi" 'evil-window-right
  "wk" 'kill-buffer-and-window
  "wm" 'evil-window-left
  "wn" 'evil-window-down
  "wo" 'delete-other-windows
  "wx" 'my/toggle-maximize-window
  "x" '(:ignore t :wk "text manipulation")
  "xC" 'my/sort-lines-by-column-reverse
  "xL" 'my/sort-lines-reverse
  "xc" 'my/sort-lines-by-column
  "xd" 'define-word-at-point
  "xl" 'my/sort-lines
  "xu" 'my/uniquify-lines)

(use-package evil :config (evil-mode +1))

The general-def leader key bindings come from my config. Obviously, there's a bunch of void functions in there, but that doesn't affect anything in this test.

Following assumes a Linux OS:

Back up your emacs folder:

mv ~/.emacs.d/ ~/.emacs.d-bak/

Paste the above init.el into ~/.emacs.d/init.el, start Emacs to let it load packages, and quit Emacs.

Then in the terminal time emacs -kill:

λ ~/ time emacs -kill
emacs -kill  2.37s user 0.03s system 99% cpu 2.416 total

Now move the (use-package evil :config (evil-mode +1)) above the general-def code block and repeat the time emacs -kill.

λ ~/ time emacs -kill
emacs -kill  0.49s user 0.03s system 82% cpu 0.622 total

Also note that if you just delete or comment out the use-package declaration for evil, the performance issue goes away as well.

If you move the evil use-package declaration back down below the key bindings and comment out half of them, then you'll see it takes ~1 second. So I'm guessing if I had twice the key bindings (or prefix keys), then it would take ~4 seconds to start emacs.

I have been working on my config applying a lot of doom style tweaks, but I've had one elephant in the room I couldn't figure out. I couldn't figure out why evil takes ~2 seconds to load for me while Doom Emacs loads it in no time. I knew it was evil because when I comment out the package everything is fast again. Or if I used use-package :defer 0.1 it would stop blocking emacs when starting up and time emacs -kill was fast again, but I wouldn't get evil mode keybindings until ~2 seconds after it was deferred.

What's worse is that this issue was flying under my radar for a while because I defer evil like this:

(use-package evil
  :hook (after-init . evil-mode)
  ;; ... elided
  )

And when you defer that way esup profiler doesn't catch it. In addition, since I run Emacs as a deamon I hardly ran into the issue.

So I went down a wild goose chase trying to figure out how Doom loaded Evil so fast. I thought it was loading it incrementally or something (which it does for ex commands). Then after exhausting that theory from looking at its code, I thought there was a performance regression in Evil-mode since the pinned version he's using and the bleeding edge version I'm on. After exhausting that theory, I finally created a new init.el with only use-package and evil and finally realized evil doesn't take as long to load as I thought. It was something else. I had to bisect my config a lot and try moving things around before I realized the performance was due to a temporal coupling between loading evil and those leader key bindings.

willbush added a commit to willbush/system that referenced this issue Apr 20, 2020
@noctuid
Copy link
Owner Author

noctuid commented May 2, 2020

This seems like an unrelated issue. It looks like evil-delay, which is what general uses for delaying keybindings if a keymap doesn't exist yet, may be slow. Generally, I recommend using general-define-key in :config or in a with-eval-after-load instead of using the builtin delaying.

Anyway, to minimize time needed for initialization and also after startup (and for fun), it seems like it would be worth it to try to create a macro version of general-define-key. Potentially I could make it so you wouldn't even need to (require 'general) if you byte-compiled your init file. I've started on a PoC. See #184.

@progfolio
Copy link

progfolio commented May 4, 2020

Just to confirm: I had a similar experience to @willbush of chasing a long init time a while back. There's a note in from my init.org which confirms I found the same performance degradation when loading general before evil. For me the load time was 7x longer when evil was loaded after general. I do use a fair amount of keymaps/prefix-commands to emulate something akin to Spacemacs, but even when culling my configuration back to the bare minimum the degradation was present.

If there's any other information I can give to help, I'm all ears.

Thank you.

@noctuid
Copy link
Owner Author

noctuid commented May 4, 2020

If there's any other information I can give to help, I'm all ears.

Was the issue loading general before evil or making evil keybindings with general before loading evil? Could you try putting the general calls before loading evil but in a (with-eval-after-load 'evil ...) and see if things are still slow?

@progfolio
Copy link

Here's a gist with my current workaround:
https://gist.github.com/progfolio/612b625afb176308c07ade86ed2c474c

If I re-order the use-package declarations (loading general before evil), Emacs will take much longer to load. As it stands I don't use the :general keyword within the evil use-package declaration. I bind to several evil commands within general's declaration. I'm not exactly sure where to put the with-eval-after-load you suggest.

@noctuid
Copy link
Owner Author

noctuid commented May 5, 2020

I bind to several evil commands within general's declaration. I'm not exactly sure where to put the with-eval-after-load you suggest.

Put it around all of the evil keybindings. For example:

(with-eval-after-load 'evil
  (global-definer ...)
   ...
  (global-definer ...))

...
(use-package evil)

@progfolio
Copy link

I see. Unfortunately, it does not seem to make a difference.

evil loaded before general (current workaround):

Emacs loaded in 2.42 seconds with 0 garbage collecitons.

general loaded before evil, evil keybindings deferred with-eval-after-load 'evil:

Emacs loaded in 10.16 seconds with 0 garbage collecitons.

@noctuid
Copy link
Owner Author

noctuid commented May 6, 2020

I've always loaded general before evil and don't see any difference in my startup time if I load it after evil. Can you replicate this without using any general functions and just loading it before evil?

@willbush
Copy link

I tested with-eval-after-load 'evil on the above test init.el file I shared and it works for me.

@noctuid
Copy link
Owner Author

noctuid commented May 10, 2020

When you say that it works, do you mean it was no longer slow?

@willbush
Copy link

Yea I mean it works the same as loading evil before doing the key bindings. It has the same performance when I tested with time emacs -kill.

@lkzz
Copy link

lkzz commented May 14, 2020

It works for me after add :demand t to use-package(emacs27).

@noctuid noctuid mentioned this issue Sep 29, 2021
@noctuid
Copy link
Owner Author

noctuid commented Apr 9, 2022

The eventual general.el rewrite will likely remove the automatic deferring functionality entirely. The current suggestion is to never use it.

The solution to the prefix issue is to use a prefix keymap where possible instead. I'm adding an FAQ entry to explain this and how to avoid other performance pitfalls (see #503).

Closing in favor of #497 and the updated FAQ.

@noctuid noctuid closed this as completed Apr 9, 2022
@noctuid noctuid reopened this Apr 19, 2022
@noctuid
Copy link
Owner Author

noctuid commented Jun 6, 2022

I reopened because there may be a better way to defer that doesn't have this issue. I will need to do some testing, but the previously proposed reason for the slowdown (extra checks for every loaded package if the keybindings should now be made) does not make sense because

  1. These checks are extremely cheap
  2. This would not cause the load time to increase with more keybindings (which I have confirmed happens for me)

My initial thought is maybe this wouldn't happen if we were storing a closure like with-eval-after-load does, but I have no idea until I test.

willbush added a commit to willbush/system that referenced this issue Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants