Possible huge setarrvalue optimization

Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

Hello,
in params.c / setarrvalue there is code:

        post_assignment_length = v->start + arrlen(val);
        if (v->end <= pre_assignment_length)
            post_assignment_length += pre_assignment_length - v->end +
            1;

        p = new = (char **) zshcalloc(sizeof(char *)
                                      * (post_assignment_length + 1));

        for (i = 0; i < v->start; i++)
            *p++ = i < pre_assignment_length ? ztrdup(*q++) :
            ztrdup("");
        for (r = val; *r;) {
            /* Give away ownership of the string */
            *p++ = *r++;
        }
        if (v->end < pre_assignment_length)
            for (q = old + v->end; *q;)
                *p++ = ztrdup(*q++);
        *p = NULL;

        v->pm->gsu.a->setfn(v->pm, new);

The point is that the code is also run when doing:

a=( a b c d )
a[1]="x"

The optimization can be: no allocation of new array when size of output
array doesn't change. The problem is that following debug:

        fprintf( _F, "%d %d\n", pre_assignment_length,
        post_assignment_length );

Shows "4 5" for the a[1]="x" assignment. Shows the same for a+=( "x" ).
If this would be fixed, one could detect that pre_assignment_length ==
post_assignment_length, and skip allocation, only do:

        for (r = val; *r;) {
            /* Give away ownership of the string */
            *p++ = *r++;
        }

This would be a huge optimization. Has anyone idea of how the
pre_assignment_length / post_assignment_length computation works, and
why it's wrong for a[1]="x" ?

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

More, the optimization would apparently allow doing the following:

typeset -a arr
max_size=100
arr[max_size]=""

for (( i=1, i<=max_size, i ++ )); do
    arr[i]="some data"
done

I was haunted by current implementation when writing morphogenesis
screen saver (https://asciinema.org/a/47242), it has arrays that
represent coordinates on screen:

https://github.com/psprint/zsh-morpho/blob/5b84d919b5bbb7183ae78635adb995ab4c3ee7da/zmorpho#L11-L15

I've had to use integer-indexed hashes instead of regular arrays, which
were very slow. After the opt I will be able to test is-at-least 5.3 and
use regular arrays, maybe even pre-allocated with the `max_size`
solution.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

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

Hello,
I attach patch that needs one problem to be resolved. Gains for
following test code:

test_fun() {
    arr[20000]=""
    repeat 10000; do
        arr[10000]="x"
    done
}

are huge: 37969 ms with no optimization, 244 ms with optimization. The
screensaver runs for 495 sec without optimization, 51 sec with opt
(still slower than hash which runs for 23 sec).

I changed:

        if (v->end <= pre_assignment_length)
            post_assignment_length += pre_assignment_length - v->end +
            1;

to:

        if (v->end <= pre_assignment_length)
            post_assignment_length += pre_assignment_length - v->end;

And this allows to test post* == pre*. I think the "+ 1" isn't needed,
v->end can be treated as "number of elements affected already".
Subtracting the number of elements from pre_assignment_length gives
correct value: elements that will be copied from old.

The thing that has to be resolved: special setters. The patch contains
one special handler tested:

if ( pre_assignment_length != post_assignment_length ||
v->pm->gsu.a->setfn == set_region_active ) {

But this doesn't work as the symbol `set_region_active` isn't available.
How to solve this, and what are other special handlers? One test is
failing (A06) – append to typeset -U array, because the special setter
isn't called.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

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

diff --git a/Src/params.c b/Src/params.c
index ef72cba..e23a0af 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2654,24 +2654,34 @@ setarrvalue(Value v, char **val)
 	    v->end = v->start;
 
 	post_assignment_length = v->start + arrlen(val);
-	if (v->end <= pre_assignment_length)
-	    post_assignment_length += pre_assignment_length - v->end + 1;
-
-	p = new = (char **) zshcalloc(sizeof(char *)
-		                      * (post_assignment_length + 1));
-
-	for (i = 0; i < v->start; i++)
-	    *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
-	for (r = val; *r;) {
-            /* Give away ownership of the string */
-	    *p++ = *r++;
-	}
-	if (v->end < pre_assignment_length)
-	    for (q = old + v->end; *q;)
-		*p++ = ztrdup(*q++);
-	*p = NULL;
 
-	v->pm->gsu.a->setfn(v->pm, new);
+	if (v->end <= pre_assignment_length)
+	    post_assignment_length += pre_assignment_length - v->end;
+
+        if ( pre_assignment_length != post_assignment_length || v->pm->gsu.a->setfn == set_region_active ) {
+            p = new = (char **) zshcalloc(sizeof(char *)
+                                          * (post_assignment_length + 1));
+
+            for (i = 0; i < v->start; i++)
+                *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
+            for (r = val; *r;) {
+                /* Give away ownership of the string */
+                *p++ = *r++;
+            }
+            if (v->end < pre_assignment_length)
+                for (q = old + v->end; *q;)
+                    *p++ = ztrdup(*q++);
+            *p = NULL;
+
+            v->pm->gsu.a->setfn(v->pm, new);
+        } else {
+            /* v->start is 0-based */
+            p = old + v->start;
+            for (r = val; *r;) {
+                /* Give away ownership of the string */
+                *p++ = *r++;
+            }
+        }
 
         /* Ownership of all strings has been
          * given away, can plainly free */

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

#!/usr/local/bin/zsh-arr-assign
#!/usr/local/bin/zsh-arr-assign-before

zmodload zsh/zprof

typeset -a arr

test_fun() {
    arr[20000]=""
    repeat 10000; do
        arr[10000]="x"
    done
}

test_fun

print -rl $arr[@] $arr[10000]

zprof

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

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

OK, the patch should now be complete. There is condition whether to use
all-duplicate code path:

        if (pre_assignment_length != post_assignment_length ||
        v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE)) {

This covers special arrays and uniq arrays, that should be always
calling setfn() to trigger side-effects. Other arrays use this code:

            /* v->start is 0-based */
            p = old + v->start;
            for (r = val; *r;) {
                /* Free previous string */
                zsfree(*p);
                /* Give away ownership of the string */
                *p++ = *r++;
            }

And it should be OK, zsfree() for non-special array should be always
successful.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

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

diff --git a/Src/params.c b/Src/params.c
index ef72cba..eac8375 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2654,24 +2654,36 @@ setarrvalue(Value v, char **val)
 	    v->end = v->start;
 
 	post_assignment_length = v->start + arrlen(val);
-	if (v->end <= pre_assignment_length)
-	    post_assignment_length += pre_assignment_length - v->end + 1;
-
-	p = new = (char **) zshcalloc(sizeof(char *)
-		                      * (post_assignment_length + 1));
-
-	for (i = 0; i < v->start; i++)
-	    *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
-	for (r = val; *r;) {
-            /* Give away ownership of the string */
-	    *p++ = *r++;
-	}
-	if (v->end < pre_assignment_length)
-	    for (q = old + v->end; *q;)
-		*p++ = ztrdup(*q++);
-	*p = NULL;
 
-	v->pm->gsu.a->setfn(v->pm, new);
+	if (v->end <= pre_assignment_length)
+	    post_assignment_length += pre_assignment_length - v->end;
+
+        if (pre_assignment_length != post_assignment_length || v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE)) {
+            p = new = (char **) zshcalloc(sizeof(char *)
+                                          * (post_assignment_length + 1));
+
+            for (i = 0; i < v->start; i++)
+                *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
+            for (r = val; *r;) {
+                /* Give away ownership of the string */
+                *p++ = *r++;
+            }
+            if (v->end < pre_assignment_length)
+                for (q = old + v->end; *q;)
+                    *p++ = ztrdup(*q++);
+            *p = NULL;
+
+            v->pm->gsu.a->setfn(v->pm, new);
+        } else {
+            /* v->start is 0-based */
+            p = old + v->start;
+            for (r = val; *r;) {
+                /* Free previous string */
+                zsfree(*p);
+                /* Give away ownership of the string */
+                *p++ = *r++;
+            }
+        }
 
         /* Ownership of all strings has been
          * given away, can plainly free */

Re: Possible huge setarrvalue optimization

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Fri, Nov 18, 2016 at 04:20:20 -0800:
> diff --git a/Src/params.c b/Src/params.c
> index ef72cba..eac8375 100644
> --- a/Src/params.c
> +++ b/Src/params.c
> @@ -2654,24 +2654,36 @@ setarrvalue(Value v, char **val)
>  	    v->end = v->start;
>  
>  	post_assignment_length = v->start + arrlen(val);
> -	if (v->end <= pre_assignment_length)
> -	    post_assignment_length += pre_assignment_length - v->end + 1;
> +	if (v->end <= pre_assignment_length)
> +	    post_assignment_length += pre_assignment_length - v->end;
> -

I'll go ahead and commit the off-by-one fix now, since it is a bugfix
independent of the performance optimisation.

> -	p = new = (char **) zshcalloc(sizeof(char *)
> -		                      * (post_assignment_length + 1));

The calloc is no longer present in latest master, so the patch failed to
apply.  (The patch did apply to 5f17007, but next time, please send
patches that apply to latest master.)

> -
> -	for (i = 0; i < v->start; i++)
> -	    *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
> -	for (r = val; *r;) {
> -            /* Give away ownership of the string */
> -	    *p++ = *r++;
> -	}
> -	if (v->end < pre_assignment_length)
> -	    for (q = old + v->end; *q;)
> -		*p++ = ztrdup(*q++);
> -	*p = NULL;
>  
> -	v->pm->gsu.a->setfn(v->pm, new);
> +
> +        if (pre_assignment_length != post_assignment_length || v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE)) {

Should this line check that «v->pm->gsu.a.setfn == arrsetfn»?

Should this line check that «pm->ename == NULL» [since arrsetfn()
handles such arrays specially]?

> +            p = new = (char **) zshcalloc(sizeof(char *)
> +                                          * (post_assignment_length + 1));
> +
> +            for (i = 0; i < v->start; i++)
> +                *p++ = i < pre_assignment_length ? ztrdup(*q++) : ztrdup("");
> +            for (r = val; *r;) {
> +                /* Give away ownership of the string */
> +                *p++ = *r++;
> +            }
> +            if (v->end < pre_assignment_length)
> +                for (q = old + v->end; *q;)
> +                    *p++ = ztrdup(*q++);
> +            *p = NULL;
> +
> +            v->pm->gsu.a->setfn(v->pm, new);
> +        } else {
> +            /* v->start is 0-based */
> +            p = old + v->start;
> +            for (r = val; *r;) {
> +                /* Free previous string */
> +                zsfree(*p);
> +                /* Give away ownership of the string */
> +                *p++ = *r++;
> +            }

Looks good.  Could you rebase to latest master please?

Cheers,

Daniel

> +        }
>  
>          /* Ownership of all strings has been
>           * given away, can plainly free */

Re: Possible huge setarrvalue optimization

[Bart Schaefer]

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

On Nov 20, 2016 3:49 AM, "Daniel Shahaf" <d.s@daniel.shahaf.name> wrote:
>
> Sebastian Gniazdowski wrote on Fri, Nov 18, 2016 at 04:20:20 -0800:
>
> > +
> > +        if (pre_assignment_length != post_assignment_length ||
v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE)) {
>
> Should this line check that «v->pm->gsu.a.setfn == arrsetfn»?
>
> Should this line check that «pm->ename == NULL» [since arrsetfn()
> handles such arrays specially]?

Thanks for pointing out those possible details.  I know from implementing
the param_private module that this part of the code is really important to
assumptions made downstream -- it may appear safe to "give away" strings
based on local examination here but in fact cause problems outside this
function.

This and the proposed getstr optimization both make me nervous.  I know
Sebastian is anxious to have them appear in the next release, but it feels
and if we should have more time using them in dev branches.

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

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

On Sun, Nov 20, 2016, at 09:41 AM, Bart Schaefer wrote:

> Thanks for pointing out those possible details.  I know from
> implementing the param_private module that this part of the code is
> really important to assumptions made downstream -- it may appear safe
> to "give away" strings based on local examination here but in fact
> cause problems outside this function.
> This and the proposed getstr optimization both make me nervous.  I
> know Sebastian is anxious to have them appear in the next release, but
> it feels and if we should have more time using them in dev branches.


Not much problem to not see this in 5.3, my code doesn't benefit much,
however it's a very cool thing that allows to do typeset -a arr;
arr[max_size] = 2000, and then freely write to not reallocated array –
really, arrays of size 5000 are unusable without the patch when storing
to single fields, times are too large. I counted on feedback about what
might break, was from some time aware of the ename, curiously tied
arrays/scalars work fine, but the environment fixing on not empty ename
is a problem.


Best regards,

Sebastian Gniazdowski

Re: Possible huge setarrvalue optimization

[Peter Stephenson]

On Sun, 20 Nov 2016 09:41:48 -0800
> This and the proposed getstr optimization both make me nervous.  I know
> Sebastian is anxious to have them appear in the next release, but it feels
> and if we should have more time using them in dev branches.

I pushed the string setting change with my tweak since it looked like it
ought to cover all possible cases sensibly.

Given what Sebastian says, we probably ought to suspend optimisations at
this point for now.

I intend to release 5.3 before Christmas come hell or high water, well,
unless it's very high, since there's plenty to get out.

pws

From zsh-workers-return-39998-mason-zsh=primenet.com.au@zsh.org Wed Nov 23 21:22:22 2016
Return-Path: <zsh-workers-return-39998-mason-zsh=primenet.com.au@zsh.org>
Delivered-To: mason-zsh@primenet.com.au
Received: (qmail 13220 invoked by alias); 23 Nov 2016 21:22:22 -0000
Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm
Precedence: bulk
X-No-Archive: yes
List-Id: Zsh Workers List <zsh-workers.zsh.org>
List-Post: <mailto:zsh-workers@zsh.org>
List-Help: <mailto:zsh-workers-help@zsh.org>
Delivered-To: mailing list zsh-workers@zsh.org
X-Seq: 39998
Received: (qmail 23583 invoked from network); 21 Nov 2016 13:46:32 -0000
X-Qmail-Scanner-Diagnostics: from mx0b-001b2d01.pphosted.com by f.primenet.com.au (envelope-from <hanpt@linux.vnet.ibm.com>, uid 7791) with qmail-scanner-2.11 
 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1.  
 Clear:RC:0(148.163.158.5):SA:0(1.1/5.0):. 
 Processed in 0.678458 secs); 21 Nov 2016 13:46:32 -0000
X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au
X-Spam-Level: *
X-Spam-Status: No, score=1.1 required=5.0 tests=LONGWORDS,RCVD_IN_DNSWL_LOW,
	RCVD_IN_MSPIKE_H2 autolearn=no autolearn_force=no version=3.4.1
X-Envelope-From: hanpt@linux.vnet.ibm.com
X-Qmail-Scanner-Mime-Attachments: |
X-Qmail-Scanner-Zip-Files: |
Received-SPF: none (ns1.primenet.com.au: domain at linux.vnet.ibm.com does not designate permitted sender hosts)
Date: Mon, 21 Nov 2016 14:50:05 +0800
From: Han Pingtian <hanpt@linux.vnet.ibm.com>
To: zsh-workers@zsh.org
Subject: Re: Remind me why ${name+word} is the way it is?
Mail-Followup-To: zsh-workers@zsh.org
References: <161111203400.ZM31567@torch.brasslantern.com>
 <20161117023325.GA3324@localhost.localdomain>
 <CAH+w=7YbPRhK1Mw0P7DKObaYU2vXCuECUGkvwp1d2R4K0dC7CA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAH+w=7YbPRhK1Mw0P7DKObaYU2vXCuECUGkvwp1d2R4K0dC7CA@mail.gmail.com>
User-Agent: Mutt/1.7.1 (2016-10-04)
X-TM-AS-GCONF: 00
X-Content-Scanned: Fidelis XPS MAILER
x-cbid: 16112106-0020-0000-0000-00000A4DB370
X-IBM-SpamModules-Scores: 
X-IBM-SpamModules-Versions: BY=3.00006115; HX=3.00000240; KW=3.00000007;
 PH=3.00000004; SC=3.00000189; SDB=6.00783291; UDB=6.00378249; IPR=6.00560910;
 BA=6.00004896; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000;
 ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013389; XFM=3.00000011;
 UTC=2016-11-21 06:50:18
X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused
x-cbparentid: 16112106-0021-0000-0000-000057708245
Message-Id: <20161121065005.GA4619@localhost.localdomain>
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-21_05:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1
 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam
 adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000
 definitions=main-1611210122

On Wed, Nov 16, 2016 at 09:34:32PM -0800, Bart Schaefer wrote:
> On Wed, Nov 16, 2016 at 6:33 PM, Han Pingtian <hanpt@linux.vnet.ibm.com> wrote:
> >
> > % name=a;echo ${name:+foo[bar]}
> > zsh: no match
> > %
> > url-quote-magic:10: no match
> 
Oh, I can still reproduce this problem by running 

    name=a;echo ${name:+foo[bar]}

two times.
> What zsh version are you using?  I can't reproduce this (tried 5.2 and
> git HEAD).
> 

I am using git HEAD, this problem can be reprodced.

> Line 10 is
> 
> words=("${(@Q)${(z)lbuf}}")
> 
> To get the "no match" message you have to have cshnullglob set, and
> obviously you also have self-insert remapped to url-quote-magic.  I
> suspect you have another setting that is influencing this.

Yes, I have cshnullglob set and self-insert remapped. If I unset
cshnullglob, then this problem cannot be reproduced any more.

Those are all the options:

% setopt
autocd
autopushd
nobgnice
braceccl
completeinword
cshnullglob
extendedglob
extendedhistory
noglobalrcs
globcomplete
histexpiredupsfirst
histfindnodups
histignorespace
histlexwords
histsavenodups
histverify
incappendhistorytime
interactive
interactivecomments
kshtypeset
nolistambiguous
magicequalsubst
monitor
pushdignoredups
rcexpandparam
rcquotes
rmstarwait
shinstdin
zle

Thanks.

Re: Possible huge setarrvalue optimization

[Daniel Shahaf]

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

Bart Schaefer wrote on Sun, Nov 20, 2016 at 09:41:48 -0800:
> This and the proposed getstr optimization both make me nervous.  I know
> Sebastian is anxious to have them appear in the next release, but it feels
> and if we should have more time using them in dev branches.

I assume we can go ahead now.

Here's a revised patch based on my review upthread:

diff --git a/Src/params.c b/Src/params.c
index 82554a7..c4dad8f 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2708,6 +2708,23 @@ setarrvalue(Value v, char **val)
 	    post_assignment_length += pre_assignment_length - v->end;
 	}
 
+	if (pre_assignment_length == post_assignment_length
+	    && v->pm->gsu.a->setfn == arrsetfn
+	    /* ... and isn't something that arrsetfn() treats specially */
+	    && 0 == (v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE))
+	    && NULL == v->pm->ename)
+    {
+	/* v->start is 0-based */
+	p = old + v->start;
+	for (r = val; *r;) {
+	    /* Free previous string */
+	    zsfree(*p);
+	    /* Give away ownership of the string */
+	    *p++ = *r++;
+	}
+    }
+	else
+    {
 	p = new = (char **) zalloc(sizeof(char *)
 		                   * (post_assignment_length + 1));
 
@@ -2726,6 +2743,7 @@ setarrvalue(Value v, char **val)
 	       post_assignment_length, (unsigned long)(p - new));
 
 	v->pm->gsu.a->setfn(v->pm, new);
+    }
 
         /* Ownership of all strings has been
          * given away, can plainly free */
@@ -3485,6 +3503,8 @@ arrsetfn(Param pm, char **x)
     /* Arrays tied to colon-arrays may need to fix the environment */
     if (pm->ename && x)
 	arrfixenv(pm->ename, x);
+    /* If you extend this function, update the list of conditions in
+     * setarrvalue(). */
 }
 
 /* Function to get value of an association parameter */

If no objections, I'll reindent and push.

Cheers,

Daniel

[-- Attachment #2: 0001-39996-Optimize-setarrvalue.patch --]
[-- Type: text/x-diff, Size: 1698 bytes --]

>From b4870d1cab9db235860adcadd821069b2e2162e9 Mon Sep 17 00:00:00 2001
From: Sebastian Gniazdowski <psprint@fastmail.com>
Date: Tue, 22 Nov 2016 09:26:46 +0000
Subject: [PATCH] AFTER RELEASE: 39996: Optimize setarrvalue().

---
 Src/params.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Src/params.c b/Src/params.c
index 82554a7..c4dad8f 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2708,6 +2708,23 @@ setarrvalue(Value v, char **val)
 	    post_assignment_length += pre_assignment_length - v->end;
 	}
 
+	if (pre_assignment_length == post_assignment_length
+	    && v->pm->gsu.a->setfn == arrsetfn
+	    /* ... and isn't something that arrsetfn() treats specially */
+	    && 0 == (v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE))
+	    && NULL == v->pm->ename)
+    {
+	/* v->start is 0-based */
+	p = old + v->start;
+	for (r = val; *r;) {
+	    /* Free previous string */
+	    zsfree(*p);
+	    /* Give away ownership of the string */
+	    *p++ = *r++;
+	}
+    }
+	else
+    {
 	p = new = (char **) zalloc(sizeof(char *)
 		                   * (post_assignment_length + 1));
 
@@ -2726,6 +2743,7 @@ setarrvalue(Value v, char **val)
 	       post_assignment_length, (unsigned long)(p - new));
 
 	v->pm->gsu.a->setfn(v->pm, new);
+    }
 
         /* Ownership of all strings has been
          * given away, can plainly free */
@@ -3485,6 +3503,8 @@ arrsetfn(Param pm, char **x)
     /* Arrays tied to colon-arrays may need to fix the environment */
     if (pm->ename && x)
 	arrfixenv(pm->ename, x);
+    /* If you extend this function, update the list of conditions in
+     * setarrvalue(). */
 }
 
 /* Function to get value of an association parameter */

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

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

-------- Original Message --------
Subject: Re: Possible huge setarrvalue optimization
Local Time: December 24, 2016 6:19 PM
UTC Time: December 24, 2016 5:19 PM
From: d.s@daniel.shahaf.name
To: Zsh hackers list <zsh-workers@zsh.org>
Sebastian Gniazdowski <psprint@fastmail.com>

Bart Schaefer wrote on Sun, Nov 20, 2016 at 09:41:48 -0800:
> This and the proposed getstr optimization both make me nervous. I know
> Sebastian is anxious to have them appear in the next release, but it feels
> and if we should have more time using them in dev branches.

I assume we can go ahead now.

Here's a revised patch based on my review upthread:

You then wrote:

"Should this line check that «v->pm->gsu.a.setfn == arrsetfn»?

Should this line check that «pm->ename == NULL» [since arrsetfn()
handles such arrays specially]?"

I think both propositions are good, especially the ename check – activated when parameter is being automatically exported – if I understand correctly. You could fix this by including code activated in the normal setter when ename is not null – like Peter did with string optimization. This way auto-exported arrays will still be optimized.

Best regards,
Sebastian Gniazdowski

Re: Possible huge setarrvalue optimization

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Wed, Jan 04, 2017 at 13:31:52 -0500:
> You then wrote:
> 
> "Should this line check that «v->pm->gsu.a.setfn == arrsetfn»?
> 
> Should this line check that «pm->ename == NULL» [since arrsetfn()
> handles such arrays specially]?"
> 
> I think both propositions are good, especially the ename check

I've incorporated them when I pushed (4fb4ce190ccb).

> – activated when parameter is being automatically exported – if I understand correctly. You could fix this by including code activated in the normal setter when ename is not null – like Peter did with string optimization. This way auto-exported arrays will still be optimized.

[Peter's change to which you refer is 39995.]

Thanks for the suggestion.  I agree the pm->ename != NULL case could be
optimised further, however, I don't intend to look into that myself.

Here's also an addendum to 39995.

diff --git a/Src/params.c b/Src/params.c
index c64d748..fc313cd 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3458,6 +3458,8 @@ strsetfn(Param pm, char *x)
 	pm->node.flags |= PM_NAMEDDIR;
 	adduserdir(pm->node.nam, x, 0, 0);
     }
+    /* If you update this function, you may need to update the
+     * `Implement remainder of strsetfn' block in assignstrvalue(). */
 }
 
 /* Function to get value of an array parameter */

Thanks again for the review.

Cheers,

Daniel

Re: Possible huge setarrvalue optimization

[Sebastian Gniazdowski]

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

-------- Original Message --------
Subject: Re: Possible huge setarrvalue optimization
Local Time: January 5, 2017 5:13 AM
UTC Time: January 5, 2017 4:13 AM
From: d.s@daniel.shahaf.name
To: Sebastian Gniazdowski <psprint@protonmail.com>
zsh-workers@zsh.org
...
Thanks for the suggestion. I agree the pm->ename != NULL case could be
optimised further, however, I don't intend to look into that myself.


Unless auto-exported parameters are marked as special (and it's rather not the case), the point is that current code will miss environment update on parameter update.

Best regards,
Sebastian Gniazdowski

Re: Possible huge setarrvalue optimization

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Thu, Jan 05, 2017 at 08:32:25 -0500:
> Unless auto-exported parameters are marked as special (and it's rather
> not the case), the point is that current code will miss environment
> update on parameter update.

Like I said, I added an 'ename == NULL' check when I pushed.  The code
in tip of master reads:

	if (pre_assignment_length == post_assignment_length
	    && v->pm->gsu.a->setfn == arrsetfn
	    /* ... and isn't something that arrsetfn() treats specially */
	    && 0 == (v->pm->node.flags & (PM_SPECIAL|PM_UNIQUE))
	    && NULL == v->pm->ename)

Is that still wrong in some way?