magit-delta icon indicating copy to clipboard operation
magit-delta copied to clipboard

Slowness relative to regular magit

Open Macavirus opened this issue 3 years ago • 37 comments

I use magit extensively and wondering if this makes any subjective performance difference vs. regular magit behaviour. If so, that would be great to see in the readme!

I think the slowdown I see regularly in magit might have something to do with the emacs fontifying/long lines issues that have been reported elswehere and nothing to do with magit itself, but unsure.

Macavirus avatar Aug 23 '20 08:08 Macavirus

Hi @Macavirus, sorry to be slow here. I don't think it does: the overhead of calling delta and converting the ANSI escape sequences to emacs text properties is, I believe, small/negligible relative to the multiple external process calls that magit makes.

My personal emacs setup does have some annoying performance issues involving magit which I haven't tracked down: in particular, I find that up/down arrow (magit-prev-line /magit-next-line) in magit diff buffers become very slow sometimes but I still haven't worked out why. (I do need to double-check that still occurs without magit-delta.)

dandavison avatar Sep 01 '20 17:09 dandavison

I find that up/down arrow (magit-prev-line /magit-next-line) in magit diff buffers become very slow sometimes but I still haven't worked out why.

(I do need to double-check that still occurs without magit-delta.)

It sounds like you might have had something specific in mind yourself though?

dandavison avatar Sep 01 '20 17:09 dandavison

@dandavison: in particular, I find that up/down arrow (magit-prev-line /magit-next-line) in magit diff buffers become very slow sometimes but I still haven't worked out why.

What does the profiler show?

mpereira avatar Sep 12 '20 10:09 mpereira

What does the profiler show?

Good question. I think I looked at this before, but I'll try to do it again when it next happens.

dandavison avatar Sep 12 '20 12:09 dandavison

OK it happened again. It seems to be saying that the time is spent in line-move-visual.

   - magit-next-line                                               35   7%
    - line-move                                                    35   7%
       line-move-visual                                            33   6%

(The percentages are low because it captured completing-read from me doing M-x profiler-stop.)

dandavison avatar Sep 26 '20 17:09 dandavison

The performance issue I saw was staging and unstaging hunks when there is a huge diff in the repo.

For example, famous package-lock.json can typically have 10,000+ lines :smiling_imp: , even though not manipulating this huge json file and it is collapsed in diff, staging other file caused 100% CPU usage and cannot recover sometimes.

braineo avatar Dec 03 '20 04:12 braineo

@braineo how is performance in the situation you describe when using magit without magit-delta?

dandavison avatar Dec 03 '20 04:12 dandavison

@dandavison

emacs 27.1.5 build from source, CPU usage is quite different only by showing magit-status without touching emacs with 2000 line difference in the repo

 package-lock.json | 2171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------
 package.json      |    2 +-

With magit-delta

image

Without magit-delta

image

(use-package magit-delta
  :disabled
  :if (executable-find "delta")
  :hook
  ((magit-mode . (lambda () (magit-delta-mode +1))))
  :config
  ;; Cannot use line number feature of delta in magit. refer to https://github.com/dandavison/magit-delta/issues/13
  (setq magit-delta-delta-args (append magit-delta-delta-args '("--features" "magit-delta"))))

.gitconfig

[interactive]
    diffFilter = delta --color-only
[delta]
    syntax-theme = Sublime Snazzy
    features = line-numbers custom-style
    whitespace-error-style = 22 reverse
[delta "custom-style"]
    plus-style = syntax "#203624"
    minus-style = syntax "#362020"
[delta "magit-delta"]
    whitespace-error-style = 22 reverse

braineo avatar Dec 03 '20 05:12 braineo

Thanks, interesting! Is it a public repo? If so a commit / any other instructions to reproduce would be great.

dandavison avatar Dec 03 '20 05:12 dandavison

That will be trivial

Any nodejs projects will do, for example

git clone [email protected]:apollographql/apollo-server.git
git reset HEAD~20

If it freezes emacs just change how many commits you want to leap back :stuck_out_tongue:

braineo avatar Dec 03 '20 05:12 braineo

It really slowed down my magit, even though the same stuff seems to work fine on command line git. had to disable the mode (these were some not-so-large .tfstate files)

sankalp-khare avatar Jan 11 '21 19:01 sankalp-khare

I had to disable it too. Having magit-delta enabled (with delta 0.6.0) caused creating magit buffers showing diffs much slower. Profiling shows that magit-git-wash is the culprit.

mpereira avatar Mar 10 '21 13:03 mpereira

The workaround I'm doing now is enabling magit-delta unless there are large diffs in the buffer:

(defvar nth/magit-delta-point-max 50000)
;; Disable mode if there are too many characters
(advice-add 'magit-delta-call-delta-and-convert-ansi-escape-sequences :around
            (defun nth/magit-delta-colorize-maybe-a (fn &rest args)
              (if (<= (point-max) nth/magit-delta-point-max)
                  (apply fn args)
                (magit-delta-mode -1))))
;; Re-enable mode after `magit-refresh' if there aren't too many characters
(add-hook 'magit-post-refresh-hook
          (defun nth/magit-enable-magit-delta-maybe-h (&rest _args)
            (when (and (not magit-delta-mode)
                       (<= (point-max) nth/magit-delta-point-max))
              (magit-delta-mode +1))))

ntharim avatar Mar 10 '21 13:03 ntharim

Sorry for not giving this much attention. It's not because I'm unaffected -- I use magit-delta daily and I encounter performance issues too (moving point in magit-status-mode slows to a crawl), but they go away on restarting Emacs (no idea why yet, but it has made me slow to get to the bottom of it).

@mpereira can you give instructions for profiling? It sounds like you have done this much more successfully than me -- I have so far failed to profile in a way that points to any relevant function.

dandavison avatar Mar 10 '21 21:03 dandavison

No worries @dandavison, and no pressure 🙂

moving point in magit-status-mode slows to a crawl

This happens to me sporadically, even without using delta or magit-delta. I consistently fix it by

  1. M-x ibuffer
  2. filtering buffers by name with the pattern "magit" (ibuffer-filter-by-name)
  3. marking all buffers for deletion (ibuffer-mark-for-delete)
  4. ibuffer-do-kill-on-deletion-marks

can you give instructions for profiling?

What I did was

  1. install and configure delta/magit-delta
  2. go to a git repository with lots of staged/unstaged changes
  3. start a profile with M-x profiler-start
  4. M-x magit-status
  5. wait for the buffer to open (depending on the diff size, it might take a while)
  6. M-x profiler-stop followed by M-x profiler-report

That should show you the function tree with the number of cycles samples collected in each function. In my case I could see magit-git-wash (which I think renders gif diffs) taking a big chunk of the time.

mpereira avatar Mar 11 '21 19:03 mpereira

Thanks a lot @mpereira. So here's the thing. I see that magit-git-wash dominates the profile timing with or without magit-delta. But I have not been able to reproduce the finding that people are reporting that magit-status is slower under magit-delta. Here's what I've tried: first a function to time it:

(defun time-magit-status ()
  (interactive)
  (let ((start-time (time-to-seconds (current-time))))
    (magit-status)
    (message "magit-status took %.1f seconds"
             (- (time-to-seconds (current-time)) start-time))))

Now, here are the results of various sequences of repeated invocations of magit-status with a fairly monstrous diff (638 files changed, 47161 insertions(+), 45836 deletions(-)). Each line is in a newly-started emacs.

magit-delta on : 10.1, 15.5, 21.1, 26.6, 33.4
magit-delta on :  9.0, 16.3, 21.3
magit-delta on : 10.3, 17.9, 24.5

magit-delta off: 11.9, 30.7, 33.6, 35.8, 36.7
magit-delta off: 11.8, 31.0, 34.7
magit-delta off: 14.9, 31.7

So there are certainly some things to explain. Why do successive calls get slower? Why is the second call in plain magit 3x slower?? And is there not a case for magit to wash these diffs lazily, and start up with the file diffs folded when there are many changed files?

But, I need someone to show how to reproduce magit-delta being slower, because I'm not managing it so far! Would someone else be able to post timings like mine above?

dandavison avatar Mar 12 '21 02:03 dandavison

Here's perhaps a slightly better way to time this:

curl -sOL https://raw.githubusercontent.com/dandavison/magit-delta/master/bin/time-magit-status
chmod +x time-magit-status

Then in a git repo with a large outstanding diff, either

USE_MAGIT_DELTA=0 time-magit-status
USE_MAGIT_DELTA=1 time-magit-status

Would anyone here be able to gather some timings on your machine, either using this script, or just in a normal emacs session using the time-magit-status function above?

With a diff of 638 changed files, I'm getting the same as above: consistently slightly faster with magit-delta. (I don't know why it would be faster; I thought that magit's elisp-based coloring algorithms were still running even when their results are going to be supplanted by magit-delta.)

dandavison avatar Mar 12 '21 03:03 dandavison

Thanks @dandavison for looking into this. My diff is 11 files changed, 1850 insertions(+), 24757 deletions(-) and using time-magit-status I got:

magit-delta off
magit-status took 0.5 seconds

magit-delta on
magit-status took 4.2 seconds

ntharim avatar Mar 12 '21 12:03 ntharim

Thanks a lot for doing that @naistran. I tried it on someone else's machine last night (and a smaller diff) and also saw slower times for magit-delta. I'm not sure yet why my set up (32 Gb macbook pro, Mitsuharu emacs-mac 27.1) is showing similar times for them.

dandavison avatar Mar 12 '21 13:03 dandavison

Here are some reproducible steps I did to get a large amount of diffs:

git clone https://github.com/NixOS/nixpkgs.git
cd nixpkgs
sed -i -e 's/nixpkgs/hello/g' $(find . -name '*.nix')
git status
curl -sOL https://raw.githubusercontent.com/dandavison/magit-delta/master/bin/time-magit-status
cat time-magit-status
chmod +x time-magit-status

results:

nixpkgs on  master [!?]
❯ USE_MAGIT_DELTA=0 ./time-magit-status
magit-delta off
magit-status took 4.2 seconds

nixpkgs on  master [!?] took 5s
❯ USE_MAGIT_DELTA=1 ./time-magit-status
magit-delta on
magit-status took 17.7 seconds

nixpkgs on  master [!?] took 19s

willbush avatar Oct 02 '21 00:10 willbush

Thanks @willbush, I ran your commands and can reproduce the slower timing with magit-delta. Here's some quick profiling results:

with magit-delta

image

without magit-delta

image

Here's one of the key magit functions involved: https://github.com/magit/magit/blob/3e415b7d533cb0a8faac696a54dbe15ee0d84bea/lisp/magit-diff.el#L2080-L2089

(defun magit-diff-wash-diffs (args &optional limit)
  (run-hooks 'magit-diff-wash-diffs-hook) ;; <=== magit-delta-call-delta-and-convert-ansi-escape-sequences
  (when (member "--show-signature" args)
    (magit-diff-wash-signature))
  (when (member "--stat" args)
    (magit-diff-wash-diffstat))
  (when (re-search-forward magit-diff-headline-re limit t)
    (goto-char (line-beginning-position))
    (magit-wash-sequence (apply-partially 'magit-diff-wash-diff args))
    (insert ?\n)))

I haven't looked into this properly but I believe that the profiling results indicate that the slowness is occurring when magit's elisp code is executing (especially magit-diff-wash-diff) but in a buffer that has been altered by magit-delta: specifically, the contents of that buffer now contain many emacs overlays representing the colors from the ANSI escape sequences, and it may be that these overlays are causing magit's processing code to be slow.

dandavison avatar Oct 02 '21 02:10 dandavison

I'm fairly new to emacs, but my setup is Doom Emacs on an Intel Macbook Pro and I notice considerably slower performance when magit-delta is enabled. I would love to use magit-delta, but I had to turn it off for some work because it was taking much longer to even pass over the (closed section) package-lock and package json files.

Mostly here to +1 the issue, but I will see if I can get some performance metrics

dodgez avatar Nov 05 '21 01:11 dodgez

Hi @tarsius, do you have a moment to advise on the performance issue we're discussing here, and maybe glance at the profiling output above (https://github.com/dandavison/magit-delta/issues/9#issuecomment-932664530)? The tentative conclusion I've reached is

I believe that the profiling results indicate that the slowness is occurring when magit's elisp code is executing (especially magit-diff-wash-diff) but in a buffer that has been altered by magit-delta: specifically, the contents of that buffer now contain many emacs overlays representing the colors from the ANSI escape sequences, and it may be that these overlays are causing magit's processing code to be slow.

To remind you of the background, magit-delta was made possible by the addition of magit-diff-wash-diffs-hook in https://github.com/magit/magit/commit/8de6ecf5c5c840f8a964c3e5bd4d7a1aedf04e10 (ref https://github.com/magit/magit/pull/4091)

dandavison avatar Nov 05 '21 14:11 dandavison

I also find it very plausible that the overlays are to be blamed.

tarsius avatar Nov 05 '21 21:11 tarsius

Thanks a lot @tarsius. Supposing that's the case, do you have any hunches or ideas about ways to improve the performance?

For example, one thing that's coming to my mind is temporarily removing the overlays and then reinstating them after magit has done its work. (But I haven't looked into that to see whether it's feasible or whether the buffer would have changed too much in the interim.)

Another thought is processing the diffs more asynchronously, e.g. using aio.el. Perhaps I could feed diff hunks one by one to the delta process, instead of processing the entire multi-file diff buffer in one go. But I don't know whether such a change could be achieved as inobtrusively as we have currently with the single hook magit-diff-wash-diffs-hook.

When developing magit-delta I did try using text-properties for the colors but IIRC I found that these interacted with text properties used by magit itself (perhaps for the hunk divider styling/folding) and thus caused a broken diff buffer. On the other hand using overlays seemed to work perfectly with essentially no work, so I went with overlays.

dandavison avatar Nov 06 '21 12:11 dandavison

I wanted to experiment a bit with this myself and get back to you after that. But I am busy with the numerous requests concerning my own packages that keep streaming in, I don't think I will get to this more complicated case any time soon.

The idea I wanted to try was doing the magit-delta thing after magit's regular diff washing instead of before. Whether that is at all feasible of course depends no whether delta adds control characters in places where it messes up regular diff washing.

tarsius avatar Nov 16 '21 14:11 tarsius

@tarsius @dandavison any further thoughts on this? This is still happening.

indigoviolet avatar Mar 08 '23 23:03 indigoviolet

At the least, it appears this issue won't be solved for a long time.

In the mean time would it be possible to have a configuration to disable magit-delta for diffs larger than a certain size?

I believe that would make it so that many users such as myself could adopt magit-delta for most diffs without negative perfomance impact.

ParetoOptimalDev avatar Jun 27 '23 14:06 ParetoOptimalDev

@ParetoOptimalDev FWIW, with large diffs, when magit says something like "Fontifying, press C-g to cancel" I often take it up on that offer. Usually what I'm trying to do is stage or commit; I wouldn't try to look at a very large diff in magit-delta, I'd always use the command line for that.

dandavison avatar Jun 27 '23 14:06 dandavison

Personally I leave delta off and toggle it on when needed:

  (defun my/magit-delta-toggle ()
    "Toggle magit-delta-mode and refresh magit."
    (interactive)
    (progn
      (call-interactively 'magit-delta-mode)
      (magit-refresh)))

willbush avatar Jun 27 '23 20:06 willbush