radian-software / straight.el

🍀 Next-generation, purely functional package manager for the Emacs hacker.
MIT License
2.72k stars 150 forks source link

Symbols created by straight.el slow down flex completion #803

Closed dsedivec closed 3 years ago

dsedivec commented 3 years ago

What's wrong

I noticed very slow flex completion and I think I traced it back to very long symbols created by straight.el. For example, one such symbol:

use-package-\(:type\ git\ :flavor\ melpa\ :files\ \(\"*groovy*\.el\"\ \"groovy-mode-pkg\.el\"\)\ :host\ github\ :repo\ \"Groovy-Emacs-Modes/groovy-emacs-modes\"\ :package\ \"groovy-mode\"\ :local-repo\ \"groovy-emacs-modes\"\)-nil-nil

Directions to reproduce

  1. Start emacs -q
  2. Eval the following to install straight and set completion-styles:
    
    (setq straight-repository-branch "develop")
    (defvar bootstrap-version)
    (let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
    (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
    (load bootstrap-file nil 'nomessage))

(setq completion-styles '(flex))


3. <kbd>M-x</kbd> `ielm` <kbd>RET</kbd>
4. Eval `(benchmark-run 1 (completion-all-completions "fooooooooooooooooooooooo" obarray #'fboundp 0))` to get a baseline.  On my system, this completes in approximately 0.04 seconds (the first number in the list returned by `benchmark-run`.  Feel free to increase 1 to a larger number to get it to run more times and get an average.  Note that the number of `o`s in `"fooooooooooooooooooooooo"` is important.  I wish I was joking about that.
5. <kbd>M-x</kbd> `straight-use-package` <kbd>RET</kbd> `groovy-mode` <kbd>RET</kbd>
6. Once again eval `(benchmark-run 1 (completion-all-completions "fooooooooooooooooooooooo" obarray #'fboundp 0))`.

Expected result: Completes in approximately 0.04 seconds once again

Observed result: Now takes 4–8 times longer, ex. 1.75 seconds

I believe these symbols are being created [here](https://github.com/raxod502/straight.el/blob/c56c56116105103ab4612b0b1f52ff685dc87bc3/straight.el#L5627-L5638).

Note that the above call to `completion-all-completions` is roughly equivalent to what [company-mode](https://company-mode.github.io/) will do in an Elisp buffer when it's using `completion-at-point` to generate completion candidates.  That's how I ran into this: I was getting *ridiculously* long pauses when writing Elisp with company-mode turned on.

I chose groovy-mode for my reproduction instructions above that's the package I hit while bisecting my `init.el` to find what was slowing down company-mode.  I'm not sure if the flex matching gets hung up because of peculiarities of the exact strings in the groovy-mode recipe (which becomes part of that `use-package-...` symbol), or if it's merely the length of the symbol (which also owes to the recipe).  I assume packages other than groovy-mode could cause this slowdown as well.

I find it unusual that you're creating such long symbols, but I think I get why you're doing it, and I don't really have any reason to believe that what you're doing is wrong or in any way a bad practice, so I understand if you wish to close this with something like "well don't use flex to complete Elisp symbols".  But I thought you would like to know that this issue could be affecting some of your other users.

If you did want to "fix" this, maybe you could use strings for the transaction IDs?  Or hash the recipe with md5 and use that?  I haven't looked real hard at straight's code.  (I'm afraid I'm burned out tonight from tracking down the source of this error!)

For my part, I think I'm going to try and advise something in the flex completion machinery not to even try and match symbols longer than a some length, and/or not to use flex when the pattern is longer than some length.

### Version information

* Emacs version: GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin20.5.0, NS appkit-2022.50 Version 11.4 (Build 20F71)) of 2021-06-19 (I built `master` earlier tonight)
* Operating system: macOS 11.4 x86-64

Thank you **so much** for all your packages, BTW!  I just realized how many I use: straight, el-patch, prescient, blackout, and ctrlf.  And now I'm looking at apheleia.
progfolio commented 3 years ago

I experience the same lag you mention w Company and Elisp buffers. Havent had a chance to look into it, but what you mention here makes sense

I'll look into this ASAP. I'm sure there's a way to avoid the long symbols.

progfolio commented 3 years ago

Hashing the recipe makes sense and actually seems to decrease start up time (marginally) on my system. I've got a patch on my fork which does so and it seems to solve the issue with Company completion. You can try it via straight-bug-report if you like:

(straight-bug-report
  :user-dir "hash-transaction-symbol.straight"
  :interactive t
  :pre-bootstrap
  (setq straight-repository-user   "progfolio"
        straight-repository-branch "fix/transaction-symbols")
  :post-bootstrap
  (setq completion-styles '(flex))
  (straight-use-package 'company))

That should open a child Emacs process which will have straight and Company installed and the flex completion style set. You can test within that environment.

I'll have to look over the transaction system more closely, but I believe these symbols may only need to last as long as a transaction. If that's true, we can unintern them at the end of a transaction as well. Will look into that ASAP.

dsedivec commented 3 years ago

Hashing the recipe makes sense and actually seems to decrease start up time (marginally) on my system. I've got a patch on my fork which does so and it seems to solve the issue with Company completion. You can try it via straight-bug-report if you like:

Tested here and indeed flex completion seems fast again! (Also, straight-bug-report is 🔥.)

I'll have to look over the transaction system more closely, but I believe these symbols may only need to last as long as a transaction. If that's true, we can unintern them at the end of a transaction as well. Will look into that ASAP.

Great, I was wondering whether straight could unintern those as well. Fewer symbols to search always helps. 😃

Thank you for looking at this!

progfolio commented 3 years ago

Glad to hear that's the case.

(Also, straight-bug-report is fire.)

Thank you. It's been an incredibly helpful tool. I have plans of making a separate, generic package that does something similar in the future.

I was wondering whether straight could unintern those as well. Fewer symbols to search always helps.

Yes, that definitely seems like the right thing to do to me. Especially considering we're using the "use-package" prefix. I've added code to unintern them at the end of each transaction. From my reading of the transaction system, I believe this is safe. I've merged the change into the "develop" branch for now so @raxod502 can have chance to look the change over. If you're not already using that branch, you can switch with:

(setq straight-repository-branch "develop")

Prior to straight's bootstrap code in your init file.

Thank you for looking at this!

Thank you for the thorough report. I had sluggish Company mode on my TODO list for quite some time, but it was never high priority. Didn't even consider straight might be causing that slowdown. I'll have to integrate more completion into my workflow now.

As always, testing is appreciated.

raxod502 commented 3 years ago

Dang, that's pretty wild as an unintended consequence. Thank you @dsedivec for the report and @progfolio for the fix. I think your change makes perfect sense, it should be fine to unintern those symbols at the end of the transaction.