Re: Possible huge setarrvalue optimization

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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 */