dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Cache the expanded prompt for editline
@ 2020-12-23 19:47 C. McEnroe
  2020-12-23 21:02 ` [PATCH v2] " C. McEnroe
  0 siblings, 1 reply; 6+ messages in thread
From: C. McEnroe @ 2020-12-23 19:47 UTC (permalink / raw)
  To: dash; +Cc: C. McEnroe

Previously, the prompt would be expanded every time editline called the
getprompt callback. I think the code may have been written assuming that
editline only calls getprompt once per prompt, but it may actually call
it many times, for instance every time you type backspace. This results
not only in slower editing from expanding complex prompts repeatedly, it
also consumes more and more stack memory each time getprompt is called.
This can be seen by setting PS1 to some command substitution, typing
many characters at the prompt, then holding backspace and observing
memory usage. Thankfully all this stack memory is freed between prompts
by the stackmark calls around el_gets.

This change causes prompt expansion to always happen in the setprompt
call, as it would when editline is disabled, and a cached copy of the
prompt is saved for getprompt to return every time editline calls it.
Since getprompt is no longer doing expansion, the stackmark calls
surrounding el_gets can be removed.
---
 src/input.c  |  6 +----
 src/parser.c | 64 ++++++++++++++++++++++++++++------------------------
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/src/input.c b/src/input.c
index 17544e7..4167bd1 100644
--- a/src/input.c
+++ b/src/input.c
@@ -152,12 +152,8 @@ retry:
 		static const char *rl_cp;
 		static int el_len;
 
-		if (rl_cp == NULL) {
-			struct stackmark smark;
-			pushstackmark(&smark, stackblocksize());
+		if (rl_cp == NULL)
 			rl_cp = el_gets(el, &el_len);
-			popstackmark(&smark);
-		}
 		if (rl_cp == NULL)
 			nr = 0;
 		else {
diff --git a/src/parser.c b/src/parser.c
index a47022e..d03697b 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -107,6 +107,9 @@ struct heredoc *heredoc;
 int quoteflag;			/* set if (part of) last token was quoted */
 
 
+static char *promptcache;
+
+
 STATIC union node *list(int);
 STATIC union node *andor(void);
 STATIC union node *pipeline(void);
@@ -1541,27 +1544,6 @@ synerror(const char *msg)
 	/* NOTREACHED */
 }
 
-STATIC void
-setprompt(int which)
-{
-	struct stackmark smark;
-	int show;
-
-	needprompt = 0;
-	whichprompt = which;
-
-#ifdef SMALL
-	show = 1;
-#else
-	show = !el;
-#endif
-	if (show) {
-		pushstackmark(&smark, stackblocksize());
-		out2str(getprompt(NULL));
-		popstackmark(&smark);
-	}
-}
-
 const char *
 expandstr(const char *ps)
 {
@@ -1611,22 +1593,23 @@ out:
 	return result;
 }
 
-/*
- * called by editline -- any expansions to the prompt
- *    should be added here.
- */
-const char *
-getprompt(void *unused)
+STATIC void
+setprompt(int which)
 {
+	struct stackmark smark;
 	const char *prompt;
+	int show;
+
+	needprompt = 0;
+	whichprompt = which;
 
 	switch (whichprompt) {
 	default:
 #ifdef DEBUG
-		return "<internal prompt error>";
+		prompt = "<internal prompt error>";
 #endif
 	case 0:
-		return nullstr;
+		prompt = nullstr;
 	case 1:
 		prompt = ps1val();
 		break;
@@ -1635,7 +1618,28 @@ getprompt(void *unused)
 		break;
 	}
 
-	return expandstr(prompt);
+#ifdef SMALL
+	show = 1;
+#else
+	show = !el;
+#endif
+	pushstackmark(&smark, stackblocksize());
+	if (show) {
+		out2str(expandstr(prompt));
+	} else {
+		free(promptcache);
+		promptcache = savestr(expandstr(prompt));
+	}
+	popstackmark(&smark);
+}
+
+/*
+ * called by editline -- return the cached prompt expansion.
+ */
+const char *
+getprompt(void *unused)
+{
+	return promptcache;
 }
 
 const char *const *
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2] Cache the expanded prompt for editline
  2020-12-23 19:47 [PATCH] Cache the expanded prompt for editline C. McEnroe
@ 2020-12-23 21:02 ` C. McEnroe
  2021-01-13  6:18   ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: C. McEnroe @ 2020-12-23 21:02 UTC (permalink / raw)
  To: dash; +Cc: C. McEnroe

Previously, the prompt would be expanded every time editline called the
getprompt callback. I think the code may have been written assuming that
editline only calls getprompt once per prompt, but it may actually call
it many times, for instance every time you type backspace. This results
not only in slower editing from expanding complex prompts repeatedly, it
also consumes more and more stack memory each time getprompt is called.
This can be seen by setting PS1 to some command substitution, typing
many characters at the prompt, then holding backspace and observing
memory usage. Thankfully all this stack memory is freed between prompts
by the stackmark calls around el_gets.

This change causes prompt expansion to always happen in the setprompt
call, as it would when editline is disabled, and a cached copy of the
prompt is saved for getprompt to return every time editline calls it.
Since getprompt is no longer doing expansion, the stackmark calls
surrounding el_gets can be removed.
---

v2: Missed adding breaks when replacing returns with assignments.

 src/input.c  |  6 +----
 src/parser.c | 66 ++++++++++++++++++++++++++++------------------------
 2 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/src/input.c b/src/input.c
index 17544e7..4167bd1 100644
--- a/src/input.c
+++ b/src/input.c
@@ -152,12 +152,8 @@ retry:
 		static const char *rl_cp;
 		static int el_len;
 
-		if (rl_cp == NULL) {
-			struct stackmark smark;
-			pushstackmark(&smark, stackblocksize());
+		if (rl_cp == NULL)
 			rl_cp = el_gets(el, &el_len);
-			popstackmark(&smark);
-		}
 		if (rl_cp == NULL)
 			nr = 0;
 		else {
diff --git a/src/parser.c b/src/parser.c
index a47022e..270a01d 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -107,6 +107,9 @@ struct heredoc *heredoc;
 int quoteflag;			/* set if (part of) last token was quoted */
 
 
+static char *promptcache;
+
+
 STATIC union node *list(int);
 STATIC union node *andor(void);
 STATIC union node *pipeline(void);
@@ -1541,27 +1544,6 @@ synerror(const char *msg)
 	/* NOTREACHED */
 }
 
-STATIC void
-setprompt(int which)
-{
-	struct stackmark smark;
-	int show;
-
-	needprompt = 0;
-	whichprompt = which;
-
-#ifdef SMALL
-	show = 1;
-#else
-	show = !el;
-#endif
-	if (show) {
-		pushstackmark(&smark, stackblocksize());
-		out2str(getprompt(NULL));
-		popstackmark(&smark);
-	}
-}
-
 const char *
 expandstr(const char *ps)
 {
@@ -1611,22 +1593,25 @@ out:
 	return result;
 }
 
-/*
- * called by editline -- any expansions to the prompt
- *    should be added here.
- */
-const char *
-getprompt(void *unused)
+STATIC void
+setprompt(int which)
 {
+	struct stackmark smark;
 	const char *prompt;
+	int show;
+
+	needprompt = 0;
+	whichprompt = which;
 
 	switch (whichprompt) {
 	default:
 #ifdef DEBUG
-		return "<internal prompt error>";
+		prompt = "<internal prompt error>";
+		break;
 #endif
 	case 0:
-		return nullstr;
+		prompt = nullstr;
+		break;
 	case 1:
 		prompt = ps1val();
 		break;
@@ -1635,7 +1620,28 @@ getprompt(void *unused)
 		break;
 	}
 
-	return expandstr(prompt);
+#ifdef SMALL
+	show = 1;
+#else
+	show = !el;
+#endif
+	pushstackmark(&smark, stackblocksize());
+	if (show) {
+		out2str(expandstr(prompt));
+	} else {
+		free(promptcache);
+		promptcache = savestr(expandstr(prompt));
+	}
+	popstackmark(&smark);
+}
+
+/*
+ * called by editline -- return the cached prompt expansion.
+ */
+const char *
+getprompt(void *unused)
+{
+	return promptcache;
 }
 
 const char *const *
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Cache the expanded prompt for editline
  2020-12-23 21:02 ` [PATCH v2] " C. McEnroe
@ 2021-01-13  6:18   ` Herbert Xu
  2021-01-14 15:43     ` June Bug
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2021-01-13  6:18 UTC (permalink / raw)
  To: C. McEnroe; +Cc: dash

C. McEnroe <june@causal.agency> wrote:
> Previously, the prompt would be expanded every time editline called the
> getprompt callback. I think the code may have been written assuming that
> editline only calls getprompt once per prompt, but it may actually call
> it many times, for instance every time you type backspace. This results
> not only in slower editing from expanding complex prompts repeatedly, it
> also consumes more and more stack memory each time getprompt is called.
> This can be seen by setting PS1 to some command substitution, typing
> many characters at the prompt, then holding backspace and observing
> memory usage. Thankfully all this stack memory is freed between prompts
> by the stackmark calls around el_gets.
> 
> This change causes prompt expansion to always happen in the setprompt
> call, as it would when editline is disabled, and a cached copy of the
> prompt is saved for getprompt to return every time editline calls it.
> Since getprompt is no longer doing expansion, the stackmark calls
> surrounding el_gets can be removed.
> ---
> 
> v2: Missed adding breaks when replacing returns with assignments.

What if someone actually wanted the prompt to change?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Cache the expanded prompt for editline
  2021-01-13  6:18   ` Herbert Xu
@ 2021-01-14 15:43     ` June Bug
  2021-01-14 20:12       ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: June Bug @ 2021-01-14 15:43 UTC (permalink / raw)
  To: Herbert Xu; +Cc: dash

> On Jan 13, 2021, at 01:18, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> What if someone actually wanted the prompt to change?

It seems unlikely given the lack of consistency of when libedit
calls the prompt callback. I only noticed it was happening when I
patched in support for EL_RPROMPT (right-aligned prompts), which
when active causes libedit to call the prompt callback on *every*
keypress. It seemed not to happen with only a regular prompt and
it took some experimenting to notice typing backspaces causes it.
The behaviour is completely undocumented in libedit.

Anecdotally, typing at the prompt becomes painful when it is expanded
over and over and does anything non-trivial, even just a command
substitution that runs no external commands.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Cache the expanded prompt for editline
  2021-01-14 15:43     ` June Bug
@ 2021-01-14 20:12       ` Herbert Xu
  2021-01-14 20:22         ` June Bug
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2021-01-14 20:12 UTC (permalink / raw)
  To: June Bug; +Cc: dash

On Thu, Jan 14, 2021 at 10:43:21AM -0500, June Bug wrote:
> 
> It seems unlikely given the lack of consistency of when libedit
> calls the prompt callback. I only noticed it was happening when I
> patched in support for EL_RPROMPT (right-aligned prompts), which
> when active causes libedit to call the prompt callback on *every*
> keypress. It seemed not to happen with only a regular prompt and
> it took some experimenting to notice typing backspaces causes it.
> The behaviour is completely undocumented in libedit.
> 
> Anecdotally, typing at the prompt becomes painful when it is expanded
> over and over and does anything non-trivial, even just a command
> substitution that runs no external commands.

OK, in that case I think it's best to fix this in libedit instead.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Cache the expanded prompt for editline
  2021-01-14 20:12       ` Herbert Xu
@ 2021-01-14 20:22         ` June Bug
  0 siblings, 0 replies; 6+ messages in thread
From: June Bug @ 2021-01-14 20:22 UTC (permalink / raw)
  To: Herbert Xu; +Cc: dash

> On Jan 14, 2021, at 15:12, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> OK, in that case I think it's best to fix this in libedit instead.

Huh? There’s no issue in libedit, only unclear documentation. The
application is the right place to store the prompt: it can be in a
static buffer, or a constant string, in which case libedit has no
need to save its own copy.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-14 20:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23 19:47 [PATCH] Cache the expanded prompt for editline C. McEnroe
2020-12-23 21:02 ` [PATCH v2] " C. McEnroe
2021-01-13  6:18   ` Herbert Xu
2021-01-14 15:43     ` June Bug
2021-01-14 20:12       ` Herbert Xu
2021-01-14 20:22         ` June Bug

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).