Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Peter Stephenson]

On Mon, 19 Jun 2017 17:16:01 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> That defeats a benefit of stdio saving read() systems calls by
> reading in chunk if we end up doing one system call per byte
> anyway.

Yes, if it's line buffered we should ideally only be unblocking
interrupts once around reading the line, which will dominate the
timing.

How about something like this?  As far as I can tell, fgets is designed
from the ground up as Gets Done Properly, so if you have it on your
system it will work correctly.  I can't think of a case where this
wouldn't do the right thing --- fgets will read at most one line and if
it does we were going to get the big STDIO overhead at that point
anyway.

pws

diff --git a/Src/input.c b/Src/input.c
index 92abaec..03d6476 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -147,6 +147,53 @@ shingetline(void)
     for (;;) {
 	winch_unblock();
 	dont_queue_signals();
+#ifdef HAVE_FGETS
+	do {
+	    errno = 0;
+	    p = fgets(buf, BUFSIZ, bshin);
+	} while (!p && errno == EINTR);
+	winch_block();
+	if (!p) {
+	    restore_queue_signals(q);
+	    return NULL;
+	}
+	c = 0;
+	while (*p) {
+	    if (imeta(STOUC(*p++)))
+		c++;
+	}
+	if (p > buf) {
+	    /* p is pointing to '\0' */
+	    ptrdiff_t nread = p - buf;
+	    queue_signals();
+	    line = zrealloc(line, ll + c + nread + 1);
+	    if (c) {
+		char *dest = line + ll;
+		char *src = buf;
+		while (src < p) {
+		    if (imeta(STOUC(*src))) {
+			*dest++ = Meta;
+			*dest++ = STOUC(*src++) ^ 32;
+		    } else
+			*dest++ = *src++;
+		}
+		*dest++ = '\0';
+	    } else {
+		/* copy null */
+		memcpy(line + ll, buf, nread + 1);
+	    }
+	    unqueue_signals();
+	    /* fgets stops at first newline but stores '\0' after */
+	    if (p[-1] == '\n') {
+		restore_queue_signals(q);
+		return line;
+	    }
+	    ll += nread;
+	} else {
+	    restore_queue_signals(q);
+	    return NULL;
+	}
+#else
 	do {
 	    errno = 0;
 	    c = fgetc(bshin);
@@ -178,6 +225,7 @@ shingetline(void)
 	    p = buf;
 	    unqueue_signals();
 	}
+#endif
     }
 }
 
diff --git a/configure.ac b/configure.ac
index 88da89e..2f19a73 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1325,7 +1325,8 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
 	       cygwin_conv_path \
 	       nanosleep \
 	       srand_deterministic \
-	       setutxent getutxent endutxent getutent)
+	       setutxent getutxent endutxent getutent \
+	       fgets)
 AC_FUNC_STRCOLL
 
 AH_TEMPLATE([REALPATH_ACCEPTS_NULL],

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Bart Schaefer]

On Jun 19,  8:28pm, Peter Stephenson wrote:
}
} How about something like this?

If that's really OK, then I think there's a simpler patch:

diff --git a/Src/input.c b/Src/input.c
index 92abaec..73e8b26 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -144,9 +144,9 @@ shingetline(void)
     int q = queue_signal_level();
 
     p = buf;
+    winch_unblock();
+    dont_queue_signals();
     for (;;) {
-	winch_unblock();
-	dont_queue_signals();
 	do {
 	    errno = 0;
 	    c = fgetc(bshin);
@@ -176,7 +176,8 @@ shingetline(void)
 	    ll += p - buf;
 	    line[ll] = '\0';
 	    p = buf;
-	    unqueue_signals();
+	    winch_unblock();
+	    dont_queue_signals();
 	}
     }
 }

-- 
Barton E. Schaefer

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Bart Schaefer]

Parsing 10,000 lines, each a colon-command followed by misc. text, and
repeated 10 times.

HEAD from git:

Src/zsh -fs  0.31s user 1.24s system 94% cpu 1.634 total
Src/zsh -fs  0.45s user 1.31s system 98% cpu 1.795 total
Src/zsh -fs  0.39s user 1.41s system 95% cpu 1.877 total
Src/zsh -fs  0.31s user 1.16s system 88% cpu 1.652 total
Src/zsh -fs  0.43s user 1.33s system 94% cpu 1.854 total
Src/zsh -fs  0.43s user 1.22s system 92% cpu 1.784 total
Src/zsh -fs  0.37s user 1.32s system 91% cpu 1.848 total
Src/zsh -fs  0.31s user 1.48s system 91% cpu 1.948 total
Src/zsh -fs  0.42s user 1.37s system 92% cpu 1.926 total
Src/zsh -fs  0.30s user 1.52s system 92% cpu 1.964 total

Peter's patch:

Src/zsh -fs  0.11s user 0.59s system 73% cpu 0.953 total
Src/zsh -fs  0.10s user 0.54s system 68% cpu 0.940 total
Src/zsh -fs  0.17s user 0.56s system 71% cpu 1.017 total
Src/zsh -fs  0.19s user 0.58s system 79% cpu 0.969 total
Src/zsh -fs  0.13s user 0.65s system 74% cpu 1.040 total
Src/zsh -fs  0.05s user 0.65s system 74% cpu 0.945 total
Src/zsh -fs  0.19s user 0.64s system 82% cpu 1.009 total
Src/zsh -fs  0.18s user 0.59s system 81% cpu 0.944 total
Src/zsh -fs  0.34s user 0.47s system 80% cpu 1.002 total
Src/zsh -fs  0.35s user 0.52s system 89% cpu 0.971 total

My "simpler" patch:

Src/zsh -fs  0.17s user 0.79s system 85% cpu 1.119 total
Src/zsh -fs  0.28s user 0.77s system 90% cpu 1.160 total
Src/zsh -fs  0.19s user 0.82s system 88% cpu 1.146 total
Src/zsh -fs  0.20s user 0.70s system 82% cpu 1.087 total
Src/zsh -fs  0.20s user 0.65s system 84% cpu 1.011 total
Src/zsh -fs  0.31s user 0.57s system 86% cpu 1.021 total
Src/zsh -fs  0.22s user 0.72s system 81% cpu 1.148 total
Src/zsh -fs  0.21s user 0.60s system 81% cpu 0.999 total
Src/zsh -fs  0.19s user 0.63s system 78% cpu 1.046 total
Src/zsh -fs  0.23s user 0.81s system 83% cpu 1.242 total

The additional speedup in PWS's patch is probably due to fewer stdio
function calls, and skip of the check for buffer overflow on every
character read.

I don't think the HAVE_FGETS + configure change is needed, there are
already at least four places where we assume fgets() is available.

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Peter Stephenson]

On Mon, 19 Jun 2017 13:38:12 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> I don't think the HAVE_FGETS + configure change is needed, there are
> already at least four places where we assume fgets() is available.

It's been there since 2001 and I added one of them myself in 2011.

pws

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Peter Stephenson]

On Mon, 19 Jun 2017 20:28:35 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> How about something like this?  As far as I can tell, fgets is designed
> from the ground up as Gets Done Properly, so if you have it on your
> system it will work correctly.  I can't think of a case where this
> wouldn't do the right thing --- fgets will read at most one line and if
> it does we were going to get the big STDIO overhead at that point
> anyway.

I know what the problem with it is --- we can't tell the difference
betwen a terminating '\0' and a '\0' that's part of the input stream.

pws

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Bart Schaefer]

On Jun 19,  9:52pm, Peter Stephenson wrote:
}
} I know what the problem with it is --- we can't tell the difference
} betwen a terminating '\0' and a '\0' that's part of the input stream.

I'll add a comment to that effect and commit my patch from w/41322.

fgets() portability (Was: Why sourcing a file is not faster than doing a loop with eval, zle -N)

[Stephane Chazelas]

2017-06-19 21:44:14 +0100, Peter Stephenson:
> On Mon, 19 Jun 2017 13:38:12 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > I don't think the HAVE_FGETS + configure change is needed, there are
> > already at least four places where we assume fgets() is available.
> 
> It's been there since 2001 and I added one of them myself in 2011.
[...]

Out of curiosity, are there really systems that have fgetc() and not
fgets()? AFAICT, both were already there in the "original"
stdio implementation in Unix v7 in the 70s.

-- 
Stephane

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Sebastian Gniazdowski]

On 20 czerwca 2017 at 01:01:26, Bart Schaefer (schaefer@brasslantern.com) wrote:
> On Jun 19, 9:52pm, Peter Stephenson wrote:
> }
> } I know what the problem with it is --- we can't tell the difference
> } betwen a terminating '\0' and a '\0' that's part of the input stream.
> 
> I'll add a comment to that effect and commit my patch from w/41322.

I have just noted a huge speed up of Zsh startup and ran bisect. I do:

repeat 10 { time /usr/local/bin/zsh -i -c exit }

and before 41322, speed is 400 ms, 350 ms when everything is compiled. With this patch, it is 220/200 ms. I thought this effect will be only for sourcing not-compiled files.

Basically this means that all the frameworks like Oh-My-Zsh can grow in size a "little" more ;)

With my hasher optimization that's 205/185 ms, but 41322 is just paradigm-shift.

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Sebastian Gniazdowski]

On 19 czerwca 2017 at 22:52:37, Peter Stephenson (p.w.stephenson@ntlworld.com) wrote:
> On Mon, 19 Jun 2017 20:28:35 +0100
> Peter Stephenson wrote:
> > How about something like this? As far as I can tell, fgets is designed
> > from the ground up as Gets Done Properly, so if you have it on your
> > system it will work correctly. I can't think of a case where this
> > wouldn't do the right thing --- fgets will read at most one line and if
> > it does we were going to get the big STDIO overhead at that point
> > anyway.
> 
> I know what the problem with it is --- we can't tell the difference
> betwen a terminating '\0' and a '\0' that's part of the input stream.

We could do a pair of calls, first fgets(), then fgetc(), and this way solve this problem?

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Sebastian Gniazdowski]

On 23 grudnia 2017 at 16:01:38, Sebastian Gniazdowski (psprint@zdharma.org) wrote:
> We could do a pair of calls, first fgets(), then fgetc(), and this way solve this problem? 

Sorry rather no, seems more complex than that.

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Mon, Mar 19, 2018 at 03:24:08PM +0100, Sebastian Gniazdowski wrote:
> So in general all this would mean that getline doesn't provide
> performance gains comparing to current HEAD?

The memory allocation that getline() does seems to slow it down
considerably; in some cases the original was actually faster.

My other guess is that since fgetc()/getc() is very common
compilers spend a lot of effort doing special optimizations for
it, versus getline() which isn't used anywhere near as much.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Sebastian Gniazdowski]

On 19 marca 2018 at 10:07:38, Joey Pabalinas (joeypabalinas@gmail.com) wrote:
> After spending a bit of time writing up a patch and testing it, getline()
> unfortunately didn't seem to solve the original performance problem

By "original problem" you mean: sig-mask being updated too often (through a syscall, like strace reported) ? AFAIR Bart patch (committed) vs. Peter patch (fgets, not commited) was resolved to: fgets is faster, but it has problems, and Bart's patch is too very fast, and correctly solves many-syscalls problem. So if you wrote the same what Peter did, but instead of fgets you used getline, then it should solve too the many-syscalls problem and be even faster than HEAD. So maybe by "original issue" you mean something else, probably: "getline should be significantly faster than current HEAD (Bart's patch commited)" ? So in general all this would mean that getline doesn't provide performance gains comparing to current HEAD?

-- 
Sebastian Gniazdowski
psprint /at/ zdharma.org

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

> On Sat, Mar 17, 2018 at 11:12:44AM +0100, Oliver Kiddle wrote:
> I didn't really follow the original discussion. But if getline() solves
> an issue and the #ifdef mess to handle old systems isn't too bad (at
> least compared to the rest of the shell) then I'd say cook up a patch.
> For compatibility, getline() is the sort of thing where we can just
> include a replacement implementation so the mess can be separate from
> the actual code. I've included a suitable implementation below.

After spending a bit of time writing up a patch and testing it, getline()
unfortunately didn't seem to solve the original performance problem in
any sort of ground-breaking way, and so after thinking it over a bit
more, I don't really think this would end up being would be any sort
of worthwhile solution.

For such a tiny performance benefit, it seem like we would be better
served looking for a solution somewhere else.

The getline() replacement implementation was very appreciated, anyway; even
though this ended up going nowhere, it saved me a day or two in getting
here :)

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Why sourcing a file is not faster than doing a loop with eval, zle -N

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On December 23, 2017 3:01 PM, Sebastian Gniazdowski wrote:
> We could do a pair of calls, first fgets(), then fgetc(), and this way
> solve this problem?

Wouldn't work because `fgets()` just returns the string as well as storing it
in the buffer instead of doing the _sane_ thing and returning the number of
characters read. Because of that it's impossible to know if there are actually
any useful characters in our buffer past the first '\0' (if you tried to read
past that you would just be asking for a buffer overrun).

Sadly, `fgets()` is of limited use for our purposes if the lines may have
embedded '\0' characters. A possible alternative is something like
POSIX `getline()` but I don't know if something like that would be kosher
for Zsh.

If it is, though, give a holler and I will cook up a patch.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]