speedup persp-maybe-kill-buffer

[mehw]

@gcv: This PR possibly fixes #168 (decreased processing speed during a persp-maybe-kill-buffer duty cycle). It may relate also to #164 and #167.

DISCLAIMER: basic-persp-killing-buffers-benchmark may crash Emacs. If it does, try to repeat the test.

In brief: persp-maybe-kill-buffer removes buffers manipulating the frame's hash table directly, and sets a perspective's dirty flag to update the windows configuration when the perspective is persp-activate re-activated.

The first commit of this PR is just a starting point I needed, a remainder from where to begin, a plain changelog update.

Switching perspective repeatedly is time consuming. In reference to perps-maybe-kill-buffer, the speed improvements gained with this PR are due to removing buffers manipulating the frame's hash table directly, rather than switching to a perspective first.

The major difference between switching-perspective-then-removing-buffer and remove-buffer-from-hash-table is that the former also updates a perspective's windows configuration.

Not updating a windows configuration means that once a perspective is switched to via persp-activate (aka re-activated), removed buffers still present in the windows configuration will be pulled back into the perspective... Unless a dirty flag is used...

Once set, a perspective's dirty flag allows to update a windows configuration only when it's time to do it, i.e. during a persp-activate cycle, buffers not belonging to the perspective (pulled back in by the windows configuration) will be forgotten by the perspective automatically.

persp-maybe-kill-buffer: different approaches, from last PR's commit to old behavior

Each section below has a descriptive header of the persp-maybe-kill-buffer behavior. Then, pseudo code and benchmark results will follow (a buffer is killed to collect the timings of kill-buffer, the persp-maybe-kill-buffer duty cycle is executed only when enabled by the persp-feature-flag-prevent-killing-last-buffer-in-perspective flag).

Benchmark creating 20 perspectives * (51 regular buffers + 51 temporary buffers). All buffers are shared between perspectives, including "*dummy*" and " *foo*". Before killig a buffer, the buffer is persp-set-buffer to the current perspective, to remove it from other perspectives (allowing it to be killed for sure, but still be part of a perspective). The sequence is:

1. Set/Unset performance flag(s) and/or enable/disable persp-maybe-kill-buffer;
2. populate with perspectives and buffers;
3. (persp-set-buffer buffer);
4. (benchmark-progn (kill-buffer buffer));
5. do the step 2 again;
6. do the step 3 again;
7. (benchmark-progn (persp-remove-buffer buffer));
8. change the conditions at the step 1;
9. run from step 2 to 9 until all possibilities are exausted.

Performance flag enabled/disabled: persp-feature-flag-directly-kill-ido-ignore-buffers

Enables/Disables persp-maybe-kill-buffer: persp-feature-flag-prevent-killing-last-buffer-in-perspective

verify buffer accessing the frame's hash table, remove from hash table and set dirty flag

... idle ...

kill-buffer -> persp-maybe-kill-buffer

read buffer

for each persp in frame's hash table {
  if should remove buffer from persp {
    if not current persp {
      (setf (persp-dirty persp) t)
      (setf (persp-buffers persp) other-buffers)
    }
  }
  if should remove buffer from current persp {
    (persp-forget-buffer buffer)
  }
  if should not keep buffer in some persp {
    return and complete kill-buffer
  }
}

... idle ...

persp-switch -> persp-activate

read persp

restore persp's buffers
restore persp's windows configuration
if persp has dirty flag set {
  for each window {
    if window's buffer not in persp {
      (persp-forget-buffer buffer)
    }
  }
}



persp-maybe-kill-buffer disabled - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.001365s: (kill-buffer "*dummy*")
Elapsed time: 0.000135s: (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.001006s: (kill-buffer "*dummy*")
Elapsed time: 0.001326s: (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer w/o performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.001060s: (kill-buffer "*dummy*")
Elapsed time: 0.001267s: (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer disabled - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000038s: (kill-buffer " *foo*")
Elapsed time: 0.000138s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000056s: (kill-buffer " *foo*")
Elapsed time: 0.000295s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer w/o performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000360s: (kill-buffer " *foo*")
Elapsed time: 0.000571s: (persp-remove-buffer " *foo*")

verify buffer accessing the frame's hash table, then switch perspectives and remove buffer

... idle ...

kill-buffer -> persp-maybe-kill-buffer

verify buffer accessing the frame's hash table

if buffer is killable {
  switching all eligible perspectives {
    (setf (persp-current-buffers) other-buffers)
  }
  return and complete kill-buffer
} else if keep buffer in some perspective {
  switching all other perspectives {
    (persp-forget-buffer buffer)
  }
  return
}



persp-maybe-kill-buffer disabled - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.001315s: (kill-buffer "*dummy*")
Elapsed time: 0.000131s: (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.434958s (0.322209s in 14 GCs): (kill-buffer "*dummy*")
Elapsed time: 0.423267s (0.309523s in 13 GCs): (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer w/o performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.435926s (0.324754s in 13 GCs): (kill-buffer "*dummy*")
Elapsed time: 0.418884s (0.305229s in 12 GCs): (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer disabled - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000024s: (kill-buffer " *foo*")
Elapsed time: 0.000128s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000052s: (kill-buffer " *foo*")
Elapsed time: 0.000271s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer w/o performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.392657s (0.283619s in 10 GCs): (kill-buffer " *foo*")
Elapsed time: 0.404069s (0.293969s in 10 GCs): (persp-remove-buffer " *foo*")

verify buffer switching perspectives, then switch perspectives and remove buffer

... idle ...

kill-buffer -> persp-maybe-kill-buffer

switch all perspectives and verify buffer

if buffer is killable {
  switching all eligible perspectives {
    (setf (persp-current-buffers) other-buffers)
  }
  return and complete kill-buffer
} else if keep buffer in some perspective {
  switching all other perspectives {
    (persp-forget-buffer buffer)
  }
  return
}



persp-maybe-kill-buffer disabled - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.001369s: (kill-buffer "*dummy*")
Elapsed time: 0.000126s: (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.404920s (0.293942s in 13 GCs): (kill-buffer "*dummy*")
Elapsed time: 0.417669s (0.304375s in 13 GCs): (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer w/o performance flag(s) - kill regular buffer
(persp-set-buffer "*dummy*")
Elapsed time: 0.435914s (0.321106s in 13 GCs): (kill-buffer "*dummy*")
Elapsed time: 0.428524s (0.312342s in 12 GCs): (persp-remove-buffer "*dummy*")

persp-maybe-kill-buffer disabled - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000030s: (kill-buffer " *foo*")
Elapsed time: 0.000140s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.000045s: (kill-buffer " *foo*")
Elapsed time: 0.000273s: (persp-remove-buffer " *foo*")

persp-maybe-kill-buffer w/o performance flag(s) - kill temporary buffer
(persp-set-buffer " *foo*")
Elapsed time: 0.432575s (0.318429s in 11 GCs): (kill-buffer " *foo*")
Elapsed time: 0.437974s (0.323620s in 11 GCs): (persp-remove-buffer " *foo*")

[gcv]

Thanks! I'll need some time to review all that.

[mehw]

Sure, take your time. Thanks, let me know.

[gcv]

@mehw: I like your approach, and as always, I'm impressed by the thoroughness of your tests.

Unfortunately, I'm running into trouble with some basics. In particular, commit 4b0ef93 introduces a problem: find-file instantly errors out.

It's reproducible on my system just by doing this:

emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'

Substitute paths as needed. I'm... not sure what's going on here:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  expand-file-name(nil)
  vc-file-getprop(nil vc-working-revision)
  vc-working-revision(nil Git)
  vc-git-mode-line-string(nil)
  apply(vc-git-mode-line-string nil)
  vc-call-backend(Git mode-line-string nil)
  vc-mode-line(nil Git)
  vc-refresh-state()
  run-hooks(find-file-hook)
  after-find-file(nil t)
  find-file-noselect-1(#<buffer perspective.el> "~/Code/perspective-el/perspective.el" nil nil "~/Code/perspective-el/perspective.el" (8689707890 16777220))
  find-file-noselect("~/Code/perspective-el/perspective.el" nil nil nil)
  find-file("~/Code/perspective-el/perspective.el")
  eval((find-file "~/Code/perspective-el/perspective.el") t)

[mehw]

Unfortunately, I'm running into trouble with some basics. In particular, commit 4b0ef93 introduces a problem: find-file instantly errors out.

It's reproducible on my system just by doing this:

emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'

@gcv: It's quite odd... thanks for bringing that out. I'm looking into it... I didn't manage to sort out the problem yet.

[mehw]

@gcv: I'm sorry for not posting for so long... I finally have time to resume working on the PR... Thank you so much for waiting patiently in these strange times... ;)

[gcv]

All good! Looking forward to reviewing your work.

[gcv]

@mehw: Do you think you'll have time to look into this?

If not, I've been using persp-feature-flag-prevent-killing-last-buffer-in-perspective and it seems to work quite well. I can flip it to on by default, and quietly get rid of it in a few weeks if no one complains.

[mehw]

@gcv: Yes, sorry, a lot has happened... I was optimistic that I could get back on track uninterrupted, but sometimes problems await us just around the corner...

I look into it. I have to rebase the PR and make the point about the error you reported:

emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'

I was able to address the problem, but still it's not clear to me the real source of it.

[mehw]

@gcv: Hi, I rebased the patch set on master, all tests passed.

About the error triggered by the below command, vc-git--run-command-string is involved, and apparently it's due to the value of current-buffer. I believe to be on a good lead, though. Thanks for the patience ;)

emacs -Q -l ~/Code/perspective-el/perspective.el --eval '(persp-mode)' --eval '(toggle-debug-on-error)' --eval '(find-file "~/Code/perspective-el/perspective.el")'

[gcv]

Okay, sounds good, let me know when you want me to take another look. Thanks for working on this.

[mehw]

Hi @gcv, I've done some progress, and things are more clear.

I need your help to decide about the appropriate behavior.

Problem

The value of the current-buffer is what was causing the problem discussed in this PR when it is changed not following the vanilla (aka persp-mode disabled) expectations in a particular scenario, involving vc-git--run-command-string, where killing any buffer would always change the current-buffer to the selected window's buffer. It's easy fixable controlling how and when the current-buffer is changed while in persp-mode (see the Bottom line below), but there could be other occasions where an unexpected change of current-buffer may give troubles...

Debugging

Right now, I'm refining the ert tests about how and when the current-buffer should be changed after any (in)direct call to persp-forget-buffer and kill-buffer (either one, persp-maybe-kill-buffer mediated and not) while in persp-mode.

Explanation

In vanilla (aka persp-mode disabled) mode, kill-buffer, when the curren-buffer is killed, would change it to the value of other-buffer; while in a perspective, this would mean that other-buffer could be outside of the perspective, selecting it from the buffer-list. Intead, bury-buffer, called with no buffer argument, would change a current-buffer, when it's also the window-buffer, to the value of the most suitable window-prev-buffers or buffer-list as fallback; while in a perspective, the chosen buffer could also be outside of the perspective when the buffer-list is considered.

Bottom line

• Should the current-buffer changes follow the vanilla expectations?
• Or, should the current-buffer become one of the current perspective's buffers only if A. it is killed, or B. it is forgot when at the same time it is in a current perspective's window?

What do you suggest?

Thank you a lot ;)

[gcv]

Does the current-buffer problem show up because some code involving vc-git-* uses kill-buffer internally on temporary buffers — which triggers all persp-related checks and special handling?

[mehw]

Yes, it does.

[gcv]

kill-buffer should always and immediately succeed on temporary buffers. That was the point of the workaround I put in:

perspective-el/perspective.el

Lines 969 to 973 in 4e38680

	;; XXX: For performance reasons, always allow killing off obviously
	;; temporary buffers. According to Emacs convention, these buffers' names
	;; start with a space.
	(when (string-match-p (rx string-start (one-or-more blank)) bufstr)
	(cl-return-from persp-maybe-kill-buffer t))

I'm not sure how current-buffer and window visibility is affected in the case of vc-git-*, though. Temporary buffers aren't supposed to go into prev and next buffer lists for a window (or if they do, that's pretty surprising behavior IMO — users aren't supposed to see them).

[mehw]

Hi @gcv, I see you removed persp-feature-flag-prevent-killing-last-buffer-in-perspective with commit e994fb3.

I plan to formalize the remaining tests this week end. I have also to fix a couple of subtle bugs ;)

[gcv]

I just renamed it and turned it on by default. Let's get more people on that code path. Looking forward to your code changes.

[mehw]

Thank you for waiting so patiently. Often I fear that the lack of new commits of mine is mistaken as lack of commitment.

[gcv]

No rush and no worries! We're all volunteers here, and this is about Emacs. It's all in good fun.