From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2923 invoked by alias); 2 Dec 2015 00:36:59 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 37275 Received: (qmail 10279 invoked from network); 2 Dec 2015 00:36:58 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-0.4 required=5.0 tests=BAYES_00,FAKE_REPLY_C, T_DKIM_INVALID autolearn=ham autolearn_force=no version=3.4.0 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= daniel.shahaf.name; h=content-type:date:from:in-reply-to :message-id:mime-version:subject:to:x-sasl-enc:x-sasl-enc; s= mesmtp; bh=eIIFZCN/K75WU/YYNY52Ot6ric0=; b=RfYqXR45NsGpZ1DUUeDAO /2Np3feN9GdiBRCyLe4j4pj52coFn0vuVrH67OR+epXhK6zuqHZHDVDHy7HNRdwr fAikaD2kvU5VQm7YbOW+y/CQttobw4xL3yCsQgQn8Qy6njWnzmv+mZsbBqF7MkPK q7AMLBTGRNRdOq2G0d2VaQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=content-type:date:from:in-reply-to :message-id:mime-version:subject:to:x-sasl-enc:x-sasl-enc; s= smtpout; bh=eIIFZCN/K75WU/YYNY52Ot6ric0=; b=FVaBk+u2hi9qZvPmiUJe wDp41Lz1coYblywpc/teMCX1AouHvSdldFufQ7c7U6pEKhgGHLFv2M91tx+DD3XA ykJ7WXlFv88KETUCjCnk2K2t4mHEiyYf6r6fSKaMJpMw2I/pGUg/a7AoMnEMj34o JL1h2IxpeP2JmnskyL4JsJE= X-Sasl-enc: t16PPIdCojDvoUPbcZ3ieucxLIz3sDapr6nEEvM30Rgw 1449016616 Date: Wed, 2 Dec 2015 00:36:54 +0000 From: Daniel Shahaf To: zsh-workers@zsh.org Subject: Re: [PATCH 3/3] Constify two local variables. Message-ID: <20151202003654.GE2462@tarsus.local2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <151130133617.ZM29077@torch.brasslantern.com> <151130123937.ZM28961@torch.brasslantern.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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