dandavison / magit-delta

Use delta (https://github.com/dandavison/delta) when viewing diffs in Magit
MIT License
295 stars 10 forks source link

Slowness relative to regular magit #9

Open Macavirus opened 4 years ago

Macavirus commented 4 years ago

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.

dandavison commented 4 years ago

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 commented 4 years ago

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?

mpereira commented 4 years ago

@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?

dandavison commented 4 years ago

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 commented 4 years ago

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.)

braineo commented 4 years ago

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.

dandavison commented 4 years ago

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

braineo commented 4 years ago

@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
dandavison commented 4 years ago

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

braineo commented 4 years ago

That will be trivial

Any nodejs projects will do, for example

git clone git@github.com: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:

sankalp-khare commented 3 years ago

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)

mpereira commented 3 years ago

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.

ntharim commented 3 years ago

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))))
dandavison commented 3 years ago

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.

mpereira commented 3 years ago

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.

dandavison commented 3 years ago

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 commented 3 years ago

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.)

ntharim commented 3 years ago

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
dandavison commented 3 years ago

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.

willbush commented 3 years ago

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
dandavison commented 3 years ago

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.

dodgez commented 3 years ago

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

dandavison commented 3 years ago

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)

tarsius commented 3 years ago

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

dandavison commented 3 years ago

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.

tarsius commented 3 years ago

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.

indigoviolet commented 1 year ago

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

ParetoOptimalDev commented 1 year ago

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.

dandavison commented 1 year ago

@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.

willbush commented 1 year ago

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)))
dandavison commented 1 year ago

I'm sorry there hasn't been any progress here! I just wanted to mention that I still use magit-delta all the time and, when it becomes slow, I restart Emacs, and that fixes it. For me it is mainly magit-next-line and magit-previous-line in the magit-status-mode buffer that I notice as becoming slow.

ParetoOptimalDev commented 1 year ago

I still use magit-delta all the time and, when it becomes slow, I restart Emacs, and that fixes it

I sometimes have many tabs opened up and don't have a stable session restoration function working yet, so that's not a good option for me at the moment.

I wonder if forcing a garbage collection is what's needed after emacs becomes slow from magit-delta though.

dandavison commented 1 year ago

I wonder if forcing a garbage collection is what's needed

Ah, good suggestion. I'll try it next time it happens and report back.

dandavison commented 11 months ago

I'm finding that M-x garbage-collect doesn't obviously fix the speed. What always fixes it for me is restarting emacs. So I'd like to know exactly what it is that is accumulating and slowing down everything in the magit-status-mode buffer. I'll try to get a sense of how long (in terms of time or number of magit-status operations) it is before it becomes noticeably slow.

willbush commented 11 months ago

@dandavison Are you saying that after viewing a lot of changes with magit-delta-mode it slows down the rest of Emacs even after you kill all buffers and garbage-collect?

dandavison commented 11 months ago

Are you saying that after viewing a lot of changes with magit-delta-mode it slows down the rest of Emacs

No, it's just operations in the magit-status-mode buffer that are slowed. I presumably don't need to restart Emacs -- I'll check that I can fix the speed by killing the buffer and restarting magit. But I only use Emacs for magit nowadays, so restarting Emacs is not an interruption for me.

even after you kill all buffers and garbage-collect?

Yes, garbage-collect doesn't fix it. I'm guessing it's something to do with the overlays and/or text-properties.

dandavison commented 11 months ago

To confirm, the gradual decrease in responsiveness that I am experiencing can be fixed by killing the magit-status-mode buffer and creating it again. I notice it particularly when trying to move point through the lines/sections of the diff (which I do with up/down arrow keys). When it has become slow, my guess is that each keypress is triggering a large number of additional operations before causing the actual buffer change and screen repaint that the keypress was intended to cause.