GNU bug report logs - #57367
[PATCH] Speed up em-smart

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57367 in the body.
You can then email your comments to 57367 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: bug-gnu-emacs <at> gnu.org
Cc: johnw <at> gnu.org
Subject: [PATCH] Speed up em-smart
Date: Tue, 23 Aug 2022 16:06:21 -0400

[Message part 1 (text/plain, inline)]

Hello,

em-smart is very slow.  This patch makes it much faster.  idk why there
was all this redisplay stuff in the module as everything seems to work
great when I remove it all.

Thanks John Wiegley for writing this module!  It's a pretty cool concept
and I'm having fun playing with it

Thanks,

Morgan

[0001-Speed-up-em-smart.patch (text/x-patch, attachment)]

Message #8 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: Jim Porter <jporterbugs <at> gmail.com>, johnw <at> gnu.org, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Sun, 04 Sep 2022 23:56:11 +0200

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> em-smart is very slow.  This patch makes it much faster.  idk why there
> was all this redisplay stuff in the module as everything seems to work
> great when I remove it all.

Skimming the patch, I think it makes sense, but I'm not that familiar
with eshell.  Perhaps Jim has some comments; added to the CCs.

Message #13 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>,
 Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: johnw <at> gnu.org, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Mon, 5 Sep 2022 12:01:21 -0700

On 9/4/2022 2:56 PM, Lars Ingebrigtsen wrote:
> Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:
> 
>> em-smart is very slow.  This patch makes it much faster.  idk why there
>> was all this redisplay stuff in the module as everything seems to work
>> great when I remove it all.
> 
> Skimming the patch, I think it makes sense, but I'm not that familiar
> with eshell.  Perhaps Jim has some comments; added to the CCs.

I don't know much about the smart scrolling module in Eshell, but I'll 
try to familiarize myself with it and then take a look. I'm slightly 
hesitant about deleting a bunch of code until I know why it was there in 
the first place; from the comments in em-smart.el, it sounds like these 
hooks are intentional:

  ;; @ While output is being generated from a command, the window will
  ;;   be constantly reconfigured (until it would otherwise make no
  ;;   difference) in order to always show you the most output from the
  ;;   command possible.  This happens if you change window sizes,
  ;;   scroll, etc.

However, maybe there's a better way to do this, or failing that, to at 
least provide some more options in the module so that users can tune the 
performance to their liking.

Message #16 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, johnw <at> gnu.org, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Mon, 05 Sep 2022 17:48:09 -0400

Hello,

Jim Porter <jporterbugs <at> gmail.com> writes:

>   ;; @ While output is being generated from a command, the window will
>   ;;   be constantly reconfigured (until it would otherwise make no
>   ;;   difference) in order to always show you the most output from the
>   ;;   command possible.  This happens if you change window sizes,
>   ;;   scroll, etc.

Yep the window is constantly reconfigured and that remains true even
with all the stuff I removed.  I only removed stuff which forced a
redisplay.  From what I can tell, we don't need to force redisplays as
everything shows up just fine.  If I did my job correctly (which I
didn't, see below) then there is zero change to the logic and user
experience.


That being said I have discovered one bug in my patch.  I use this
little tid bit to determine if I should scroll or not in the
eshell-output-filter-functions hook.

"(when (eq (window-buffer (selected-window)) (current-buffer)) <logic here>)"

Whereas the previous code used this logic in a few hooks:

(walk-windows
 (lambda (wind)
   (with-current-buffer (window-buffer wind)
     (if eshell-mode
         <logic here>)))
 0 frame)


My code prevents the hook from being run on buffers that aren't the
eshell buffer (no clue how but it does) but it doesn't allow the hook to
run if the window isn't selected.  The old code seems very complicated
though and will scroll all eshell buffers.

Is there a good way to ensure a hook is performed only on the buffer
that it was installed to?


Thanks,

Morgan

Message #19 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: Jim Porter <jporterbugs <at> gmail.com>, johnw <at> gnu.org, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Mon, 05 Sep 2022 23:51:28 +0200

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> Is there a good way to ensure a hook is performed only on the buffer
> that it was installed to?

`add-hook' takes a LOCAL parameter.  (But I haven't looked at the logic
here, so perhaps something else is needed.)

Message #22 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Tue, 06 Sep 2022 12:00:26 +0200

(Please keep the debbugs address in the CCs -- otherwise it won't reach
the bug tracker.)

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> So the hook is local to the buffer but it runs this bit of code:
>
> (set-window-start (selected-window) (line-beginning-position) t)
>
> So I want to set all windows that refer to the hook buffer to start at a
> certain location.  Ideally I want to do this even if the window doesn't
> currently exist as I want it to be properly scrolled when I get back to
> it.

Hm, right.

> I guess that explains why the original author simply walked through
> all windows and also attached the hook to
> `window-configuration-change-hook'.  I guess I could restore the
> original solution but it doesn't seem particularly elegant.
>
> Let me know if you have a better idea, otherwise I'll simply send
> another patch that puts some of the old stuff back in.

I think you probably have to wait until the window actually exists to
effect scrolling in it (you may have the same buffer in several windows,
or in none), really, so I think walking the windows is the only
practical solution here.

But there may be other tricks here -- perhaps somebody else has some
ideas.

Message #25 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH V2] Speed up em-smart
Date: Tue, 06 Sep 2022 21:30:18 -0400

[Message part 1 (text/plain, inline)]

Hello,

I've attached my patch V2.

This restores some more of the original logic that I have now realized
was indeed necessary.  Again, this patch should not actually change
anything with respect to program logic, flow, or user experience.  In my
limited testing, it seems to act just as it did before (but
significantly more performant).

[0001-Speed-up-em-smart.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> (Please keep the debbugs address in the CCs -- otherwise it won't reach
> the bug tracker.)

Sorry, still trying to get the hang of gnus :P


Thanks,

Morgan

Message #28 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH V2] Speed up em-smart
Date: Wed, 07 Sep 2022 14:55:19 +0200

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> I've attached my patch V2.
>
> This restores some more of the original logic that I have now realized
> was indeed necessary.  Again, this patch should not actually change
> anything with respect to program logic, flow, or user experience.  In my
> limited testing, it seems to act just as it did before (but
> significantly more performant).

It think that makes sense...  Jim, any comments?

Message #31 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH V2] Speed up em-smart
Date: Thu, 8 Sep 2022 21:36:17 -0700

On 9/6/2022 6:30 PM, Morgan Smith wrote:
> I've attached my patch V2.
> 
> This restores some more of the original logic that I have now realized
> was indeed necessary.  Again, this patch should not actually change
> anything with respect to program logic, flow, or user experience.  In my
> limited testing, it seems to act just as it did before (but
> significantly more performant).

Thanks, this does indeed seem a lot faster. I do notice one small change 
in behavior though: after starting Eshell with the em-smart module 
loaded, run "echo hi" and then split the window vertically with 'C-x 2'. 
Before the patch, the window doesn't scroll. After the patch, the window 
is scrolled so that the "echo hi" block is at the top (i.e. the "Welcome 
to the Emacs shell" banner is hidden).

The same sort of thing happens if you run a command with a lot of output 
first and then run "echo hi". Before splitting, the "echo hi" block is 
at the bottom. Without this patch, 'C-x 2' maintains that position (i.e. 
it scrolls the minimum amount of the previous command out of view so 
that you can see the most-recent command). With the patch, 'C-x 2' 
scrolls *all* of the previous command out of view, so the "echo hi" 
block is at the top.

I think the pre-patch behavior is the most-usable: if you can fit all of 
the last command in the window, it should be at the bottom so that you 
can see (some of) the previous command.

(I also saw some strange issue with resizing the windows via the mouse, 
but I can't reproduce it anymore. If I can figure out how trigger this 
reliably, I'll send an update. Sorry that's not very helpful at present...)

Message #34 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Morgan Smith <Morgan.J.Smith <at> outlook.com>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH] Speed up em-smart
Date: Wed, 6 Sep 2023 15:46:28 -0700

Jim Porter <jporterbugs <at> gmail.com> writes:

> On 9/6/2022 6:30 PM, Morgan Smith wrote:
>> I've attached my patch V2.
>> This restores some more of the original logic that I have now realized
>> was indeed necessary.  Again, this patch should not actually change
>> anything with respect to program logic, flow, or user experience.  In my
>> limited testing, it seems to act just as it did before (but
>> significantly more performant).
>
> Thanks, this does indeed seem a lot faster. I do notice one small change in
> behavior though: after starting Eshell with the em-smart module loaded, run
> "echo hi" and then split the window vertically with 'C-x 2'. Before the patch,
> the window doesn't scroll. After the patch, the window is scrolled so that the
> "echo hi" block is at the top (i.e. the "Welcome to the Emacs shell" banner is
> hidden).
>
> The same sort of thing happens if you run a command with a lot of output first
> and then run "echo hi". Before splitting, the "echo hi" block is at the
> bottom. Without this patch, 'C-x 2' maintains that position (i.e. it scrolls the
> minimum amount of the previous command out of view so that you can see the
> most-recent command). With the patch, 'C-x 2' scrolls *all* of the previous
> command out of view, so the "echo hi" block is at the top.
>
> I think the pre-patch behavior is the most-usable: if you can fit all of the
> last command in the window, it should be at the bottom so that you can see (some
> of) the previous command.
>
> (I also saw some strange issue with resizing the windows via the mouse, but I
> can't reproduce it anymore. If I can figure out how trigger this reliably, I'll
> send an update. Sorry that's not very helpful at present...)

That was one year ago.

Morgan, did you get a chance to look into the issues Jim pointed out
above?

Message #37 received at 57367 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Jim Porter <jporterbugs <at> gmail.com>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 57367 <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH v3] Speed up em-smart
Date: Wed, 18 Oct 2023 11:46:57 -0400

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Jim Porter <jporterbugs <at> gmail.com> writes:
>
>> On 9/6/2022 6:30 PM, Morgan Smith wrote:
>>> I've attached my patch V2.
>>> This restores some more of the original logic that I have now realized
>>> was indeed necessary.  Again, this patch should not actually change
>>> anything with respect to program logic, flow, or user experience.  In my
>>> limited testing, it seems to act just as it did before (but
>>> significantly more performant).
>>
>> Thanks, this does indeed seem a lot faster. I do notice one small change in
>> behavior though: after starting Eshell with the em-smart module loaded, run
>> "echo hi" and then split the window vertically with 'C-x 2'. Before the patch,
>> the window doesn't scroll. After the patch, the window is scrolled so that the
>> "echo hi" block is at the top (i.e. the "Welcome to the Emacs shell" banner is
>> hidden).
>>
>> The same sort of thing happens if you run a command with a lot of output first
>> and then run "echo hi". Before splitting, the "echo hi" block is at the
>> bottom. Without this patch, 'C-x 2' maintains that position (i.e. it scrolls the
>> minimum amount of the previous command out of view so that you can see the
>> most-recent command). With the patch, 'C-x 2' scrolls *all* of the previous
>> command out of view, so the "echo hi" block is at the top.
>>
>> I think the pre-patch behavior is the most-usable: if you can fit all of the
>> last command in the window, it should be at the bottom so that you can see (some
>> of) the previous command.
>>
>> (I also saw some strange issue with resizing the windows via the mouse, but I
>> can't reproduce it anymore. If I can figure out how trigger this reliably, I'll
>> send an update. Sorry that's not very helpful at present...)
>
> That was one year ago.
>
> Morgan, did you get a chance to look into the issues Jim pointed out
> above?

Thanks for the ping.  Reading the documentation for
`window-configuration-change-hook' I found out I can run the scroll
command only on updated windows.  Furthermore, that hook selects the
window which is nice.  I believe the problems pointed out above stem
from using `(point)` to scroll a window that wasn't actually selected.

Anyways here is V3

[v3-0001-Speed-up-em-smart.patch (text/x-patch, attachment)]

Message #42 received at 57367-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>,
 Stefan Kangas <stefankangas <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57367-done <at> debbugs.gnu.org
Subject: Re: bug#57367: [PATCH v3] Speed up em-smart
Date: Sat, 28 Oct 2023 15:47:52 -0700

Version: 30.1

On 10/18/2023 8:46 AM, Morgan Smith wrote:
> Thanks for the ping.  Reading the documentation for
> `window-configuration-change-hook' I found out I can run the scroll
> command only on updated windows.  Furthermore, that hook selects the
> window which is nice.  I believe the problems pointed out above stem
> from using `(point)` to scroll a window that wasn't actually selected.
> 
> Anyways here is V3

Thanks for the updated patch. As far as I can tell (I don't use the 
smart display module in Eshell), everything works here, and you even 
fixed a bug I noticed on master with long output! (Previously, if you 
ran a command with a lot of output, the prompt got hidden for some 
reason, probably due to using field properties for the prompt/output. 
That's fixed now.)

I've merged this to master as e08238cdd74 now. Closing this bug. Thanks 
again.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.