Re: [PATCH 3/3] Constify two local variables.

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Bart Schaefer wrote on Mon, Nov 30, 2015 at 12:39:37 -0800:
> On Nov 30,  4:42pm, Peter Stephenson wrote:
> }
> } As Bart says, setfn() expects a permanently allocated values; if it
> } doesn't neeed it, it's up to the setfn in question to convert the value
> } and then free what's passed in.
> 
> Thinking about it, it is quite likely that this makes it impossible to
> optimize in the way Daniel wants.  At a very low level, we can't swap
> values into an existing (char**) representing an array parameter.  All
> the setfn() implementations would have to be updated to notice when
> the pointer passed in is the same as the pointer already in the param
> struct and skip the free, or something like that.

There's a sort of a third way here: I could change

    vals = getfn()
    munge_in_place(vals)
    setfn(vals)

to

    #define N (sizeof(void*) * arrlen(vals))
    vals = getfn()
    munge_in_place(vals)
    dummy = zalloc(N)
    memcpy(dummy, vals, N)
    setfn(dummy)

which still does O(N) work, but does _less_ work: it still allocates and
initializes one chunk of N pointers, but (compared to the setarrvalue()
in HEAD) doesn't allocate and initialize N chunks of one element each.

Bart Schaefer wrote on Mon, Nov 30, 2015 at 13:36:17 -0800:
> On Nov 30, 12:39pm, Bart Schaefer wrote:
> } Subject: Re: [PATCH 3/3] Constify two local variables.
> }
> } All the setfn() implementations would have to be updated to notice when
> } the pointer passed in is the same as the pointer already in the param
> } struct and skip the free, or something like that.
> 
> Hmm, the default array and hash setfn() already do that.  So it would
> only be an issue with some of the specials?

Specials, and module-defined parameters that don't use arrsetfn(),
I believe.

So, one option is to change all setfn()s to support the setfn(getfn())
invocation...?  But that'll mean modules written for 5.1.2 would have to
be *patched*, not just recompiled, to work with 5.1.3, unless we put in
a way for Param's to indicate whether they support the setfn(getfn())
invocation (to let core skip the optimization for Param's that don't
support that).

But in the meantime, we can just limit the optimization to Param's that
are known to support it:

/* to be applied on top of 37257 (which itself is to be applied on top of 37253) */
diff --git a/Src/params.c b/Src/params.c
index 94c996a..1372958 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2579,7 +2579,11 @@ setarrvalue(Value v, char **val)
        if (v->end <= pre_assignment_length)
            post_assignment_length += pre_assignment_length - v->end;
 
-       if (post_assignment_length == pre_assignment_length) {
+       if (post_assignment_length == pre_assignment_length &&
+           v->pm->gsu.a->setfn == arrsetfn) {
            /* Optimization: avoid reallocating. */
+           /* Only enable the optimization for setfn's that support being called
+            * on the return value of getfn, such as arrsetfn. */
+
            const int effective_end_index = MIN(v->end, pre_assignment_length);
            int i;

This fixes the SIGABRT, I think I can commit it like this.  So, I'll
commit 37253 and 37257+this after the release. 

Now, one of my actual use-cases for this happens to be region_highlight
(for zsh-syntax-highlighting reasons), which doesn't use arrsetfn, but
I'll see about that separately...

Cheers,

Daniel