speedup persp-maybe-kill-buffer #171

mehw · 2021-12-02T11:16:02Z

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

Set/Unset performance flag(s) and/or enable/disable persp-maybe-kill-buffer;
populate with perspectives and buffers;
(persp-set-buffer buffer);
(benchmark-progn (kill-buffer buffer));
do the step 2 again;
do the step 3 again;
(benchmark-progn (persp-remove-buffer buffer));
change the conditions at the step 1;
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*")

Expect that temporary buffers are 'ido-ignore-buffers'. Use 'persp--make-ignore-buffer-rx' to detect them. This is also implemented in other contexts of perspective.el.

Customizable variable to allow a 'persp-maybe-kill-buffer' call to kill 'ido-ignore-buffers' (aka temporary buffers) skipping checks. Temproray buffers usually have a name that starts with a space.

Usually temporary buffers have a name that starts with a space. Set 'ido-ignore-buffers' to a regular expression to detect that in ert's tests.

Benchmark 'persp-maybe-kill-buffer'.

Read perspectives' buffers directly accessing the frame's hash table for performance reasons.

Do not remove buffers from perspectives via 'with-perspective' macro. It takes a heavy toll when switching perspective. Access the frame's hash table directly instead. This improves performance but doesn't update a windows configuration, like properly removing a buffer from the current perspective does.

Due to 'persp-maybe-kill-buffer' amendments, force update the 'current-buffer' to the current window's buffer after calling 'kill-buffer'.

Implement a dirty flag to let a perspective forget buffers which are found in windows, but do not belong to the perspective. A buffer removed from a perspective by manipulating the frame's hash table, requires the perspective's dirty flag to be set. This is required since 'persp-maybe-kill-buffer' no longer updates a perspective's windows configuration. When a removed buffer is still part of a windows configuration, a perspective will get back removed buffers when re-activated.

Unset feature flag by default, due to nex3#168 possibly beeing fixed.

Set feature flag by default, due to nex3#168 possibly beeing fixed.

gcv · 2021-12-02T16:02:21Z

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

mehw · 2021-12-02T17:33:44Z

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

gcv · 2021-12-13T06:12:36Z

@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 · 2021-12-15T09:19:21Z

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 · 2022-02-01T15:45:49Z

@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 · 2022-02-01T20:29:52Z

All good! Looking forward to reviewing your work.

gcv · 2022-04-22T05:23:25Z

@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 · 2022-04-22T14:53:58Z

@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 · 2022-05-04T16:26:23Z

@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 · 2022-05-05T04:39:09Z

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

mehw · 2022-05-17T17:00:54Z

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 · 2022-05-19T00:57:19Z

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 · 2022-05-19T13:46:19Z

Yes, it does.

gcv · 2022-05-19T14:00:30Z

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 · 2022-07-06T13:42:25Z

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 · 2022-07-06T14:11:35Z

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 · 2022-07-07T10:56:46Z

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

gcv · 2022-07-09T03:26:37Z

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

mehw added 4 commits December 2, 2021 05:20

Update changelog

868f27f

persp-maybe-kill-buffer: directly kill ido-ignore-buffers

ae8346f

Expect that temporary buffers are 'ido-ignore-buffers'. Use 'persp--make-ignore-buffer-rx' to detect them. This is also implemented in other contexts of perspective.el.

persp-feature-flag-directly-kill-ido-ignore-buffers: new flag

d3a20cd

Customizable variable to allow a 'persp-maybe-kill-buffer' call to kill 'ido-ignore-buffers' (aka temporary buffers) skipping checks. Temproray buffers usually have a name that starts with a space.

ert tests: set ido-ignore-buffers to detect temporary buffers

127eb39

Usually temporary buffers have a name that starts with a space. Set 'ido-ignore-buffers' to a regular expression to detect that in ert's tests.

mehw force-pushed the speedup_persp-maybe-kill-buffer branch from 33b4a2f to 691eceb Compare December 2, 2021 12:41

mehw added 7 commits December 2, 2021 13:45

basic-persp-killing-buffers-benchmark: new ert test

764080d

Benchmark 'persp-maybe-kill-buffer'.

persp-maybe-kill-buffer: read buffers via the frame's hash table

797961e

Read perspectives' buffers directly accessing the frame's hash table for performance reasons.

persp-maybe-kill-buffer-adv: force update current-buffer

4b0ef93

Due to 'persp-maybe-kill-buffer' amendments, force update the 'current-buffer' to the current window's buffer after calling 'kill-buffer'.

persp-feature-flag-directly-kill-ido-ignore-buffers: unset

82d9b8b

Unset feature flag by default, due to nex3#168 possibly beeing fixed.

persp-feature-flag-prevent-killing-last-buffer-in-perspective: set

0813f6a

Set feature flag by default, due to nex3#168 possibly beeing fixed.

mehw force-pushed the speedup_persp-maybe-kill-buffer branch from 691eceb to 0813f6a Compare December 2, 2021 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

speedup persp-maybe-kill-buffer #171

speedup persp-maybe-kill-buffer #171

mehw commented Dec 2, 2021 •

edited

gcv commented Dec 2, 2021

mehw commented Dec 2, 2021

gcv commented Dec 13, 2021

mehw commented Dec 15, 2021

mehw commented Feb 1, 2022

gcv commented Feb 1, 2022

gcv commented Apr 22, 2022

mehw commented Apr 22, 2022

mehw commented May 4, 2022

gcv commented May 5, 2022

mehw commented May 17, 2022 •

edited

gcv commented May 19, 2022

mehw commented May 19, 2022

gcv commented May 19, 2022

mehw commented Jul 6, 2022

gcv commented Jul 6, 2022

mehw commented Jul 7, 2022

gcv commented Jul 9, 2022

speedup persp-maybe-kill-buffer #171

Are you sure you want to change the base?

speedup persp-maybe-kill-buffer #171

Conversation

mehw commented Dec 2, 2021 • edited

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

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

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

verify buffer switching perspectives, then switch perspectives and remove buffer

gcv commented Dec 2, 2021

mehw commented Dec 2, 2021

gcv commented Dec 13, 2021

mehw commented Dec 15, 2021

mehw commented Feb 1, 2022

gcv commented Feb 1, 2022

gcv commented Apr 22, 2022

mehw commented Apr 22, 2022

mehw commented May 4, 2022

gcv commented May 5, 2022

mehw commented May 17, 2022 • edited

Problem

Debugging

Explanation

Bottom line

gcv commented May 19, 2022

mehw commented May 19, 2022

gcv commented May 19, 2022

mehw commented Jul 6, 2022

gcv commented Jul 6, 2022

mehw commented Jul 7, 2022

gcv commented Jul 9, 2022

mehw commented Dec 2, 2021 •

edited

mehw commented May 17, 2022 •

edited