[PATCH] getstrvalue() optimization

[PATCH] getstrvalue() optimization

[Sebastian Gniazdowski]

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

Hello
There are double strlen() invocations in getstrvalue(). It's not about
negative indexes – although they have been optimized too – but about
dupstring() and v->end, v->start verification code at the end. Actually
a single strlen() call is required. Attached test script runs 1765 ms
for optimized Zsh, 1980 ms for no optimizations (minimum obtainable
times):

strtest() {
    a=""
    i=$(( 4000 ))
    while (( i -- )); do
        a+="a${a[1,200]}"
    done
}

I've used signed size variable but the code did that already. More
optimization is possible – dupstring() could do strncpy(), copy only
requested number of bytes, e.g. first 200 out of 10000, but the trailing
\0 nuances made me wait with this change.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

[-- Attachment #2: string_opt.diff --]
[-- Type: text/plain, Size: 1504 bytes --]

diff --git a/Src/params.c b/Src/params.c
index 330f22b..5005042 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2060,6 +2060,7 @@ getstrvalue(Value v)
 {
     char *s, **ss;
     char buf[BDIGBUFSIZE];
+    int len = -1;
 
     if (!v)
 	return hcalloc(1);
@@ -2237,22 +2238,26 @@ getstrvalue(Value v)
 	return s;
 
     if (v->start < 0) {
-	v->start += strlen(s);
+	v->start += (len=(int)strlen(s));
 	if (v->start < 0)
 	    v->start = 0;
     }
     if (v->end < 0) {
-	v->end += strlen(s);
+        len = (len >= 0) ? len : (int)strlen(s);
+	v->end += len;
 	if (v->end >= 0) {
 	    char *eptr = s + v->end;
 	    if (*eptr)
 		v->end += MB_METACHARLEN(eptr);
 	}
     }
-    s = (v->start > (int)strlen(s)) ? dupstring("") : dupstring(s + v->start);
+
+    len = (len >= 0) ? len : (int)strlen(s);
+    s = (v->start > len) ? dupstring("") : dupstring_wlen(s + v->start, len - v->start);
+
     if (v->end <= v->start)
 	s[0] = '\0';
-    else if (v->end - v->start <= (int)strlen(s))
+    else if (v->end - v->start <= len - v->start)
 	s[v->end - v->start] = '\0';
 
     return s;
diff --git a/Src/string.c b/Src/string.c
index 04e7446..b46ea60 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -43,6 +43,19 @@ dupstring(const char *s)
 
 /**/
 mod_export char *
+dupstring_wlen(const char *s, unsigned len)
+{
+    char *t;
+
+    if (!s)
+	return NULL;
+    t = (char *) zhalloc(len + 1);
+    strcpy(t, s);
+    return t;
+}
+
+/**/
+mod_export char *
 ztrdup(const char *s)
 {
     char *t;

[-- Attachment #3: testopt2.zsh --]
[-- Type: application/octet-stream, Size: 213 bytes --]

#!Src/zsh-opt3
#!Src/zsh-no-opt
#!Src/zsh-opt2
#!Src/zsh-opt

zmodload zsh/zprof

strtest() {
    a=""

    i=$(( 4000 ))
    while (( i -- )); do
        a+="a${a[1,200]}"
    done
}

strtest

zprof | head -n 25

Re: [PATCH] getstrvalue() optimization

[Peter Stephenson]

While I'm still looking at this... We always need the string length;
there's no early return between the first two cases and where we ensure
we have it.

pws

diff --git a/Src/params.c b/Src/params.c
index 772345b..ef72cba 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2060,7 +2060,7 @@ getstrvalue(Value v)
 {
     char *s, **ss;
     char buf[BDIGBUFSIZE];
-    int len = -1;
+    int len;
 
     if (!v)
 	return hcalloc(1);
@@ -2237,15 +2237,13 @@ getstrvalue(Value v)
     if (v->start == 0 && v->end == -1)
 	return s;
 
+    len = strlen(s);
     if (v->start < 0) {
-	len = strlen(s);
 	v->start += len;
 	if (v->start < 0)
 	    v->start = 0;
     }
     if (v->end < 0) {
-	if (len < 0)
-	    len = strlen(s);
 	v->end += len;
 	if (v->end >= 0) {
 	    char *eptr = s + v->end;
@@ -2254,8 +2252,6 @@ getstrvalue(Value v)
 	}
     }
 
-    if (len < 0)
-	len = strlen(s);
     s = (v->start > len) ? dupstring("") :
 	dupstring_wlen(s + v->start, len - v->start);
 

Re: [PATCH] getstrvalue() optimization

[Sebastian Gniazdowski]

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

Hello,
I thought to make some things clear, not sure if without this any
problems are possible (tried to obtain bad behavior but Zsh worked
normally).

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

[-- Attachment #2: string_fix.diff --]
[-- Type: text/plain, Size: 663 bytes --]

diff --git a/Src/params.c b/Src/params.c
index ef72cba..0d17047 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2060,7 +2060,7 @@ getstrvalue(Value v)
 {
     char *s, **ss;
     char buf[BDIGBUFSIZE];
-    int len;
+    int len, zerol=0;
 
     if (!v)
 	return hcalloc(1);
@@ -2255,9 +2255,12 @@ getstrvalue(Value v)
     s = (v->start > len) ? dupstring("") :
 	dupstring_wlen(s + v->start, len - v->start);
 
+    if (s[0] == '\0')
+        zerol = 1;
+
     if (v->end <= v->start)
 	s[0] = '\0';
-    else if (v->end - v->start <= len - v->start)
+    else if (!zerol && v->end - v->start <= len - v->start)
 	s[v->end - v->start] = '\0';
 
     return s;

Re: [PATCH] getstrvalue() optimization

[Peter Stephenson]

On Thu, 10 Nov 2016 00:14:24 -0800
Sebastian Gniazdowski <psprint@fastmail.com> wrote:
> Hello,
> I thought to make some things clear, not sure if without this any
> problems are possible (tried to obtain bad behavior but Zsh worked
> normally).

I don't think this needs a special case.  If it did, I think that
would mean some assumption in the code was already broken.

pws