[perl #120939] [PATCH] refactor and fix leak in const_av_xsub

On Thu Apr 03 23:57:57 2014, bulk88 wrote:
> On Sun Mar 23 16:38:17 2014, tonyc wrote:
> > On Sun Mar 09 22:28:10 2014, bulk88 wrote:
> > > On Tue Feb 11 22:16:17 2014, tonyc wrote:
> > > > - avoid doing the same gimme test twice, which could confuse a
> > > > reader
> > >
> > > Now there are 2 EXTENDs which are quite fat (for C) to do, scaling,
> > > 4
> > > pushes, comparison, reading interp globals. My code has just 1
> > > EXTEND
> > > call, which I like.
> >
> > The perl implementation strikes the speed vs size balance heavily on
> > the speed side - using macros and inline functions which produce
> > faster but significantly larger code.
> 
> Which slows you down when you have a 1 KB in the machine code function
> to execute. Or 25 calls to hv_common_key() when someone put it inside
> SvTRUE.

In this specific case, on a compiler we support UNLIKELY() with, the unlikely code will be moved out of the flow of execution and will have little to no effect on speed.

> 
> >
> > So I think adding code like:
> >
> > const SSize_t stack_grow = 1 + (-(gimme_v == G_ARRAY) & avlastindex);
> >
> > to save a few bytes from the extra EXTEND() expansion (while slighly
> > slowing it down) doesn't match how perl's implementation has balanced
> > speed vs size.
> 
> That is trading size for speed. I can write that as a traditional if()
> {} if you want me to. It will be less machine code, but then its a
> branch/conditional jump (slower).

That won't help enough for me to accept this patch.

BTW:

  if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);

should probably be:

  assert(gimme_v != G_ARRAY || stack_grow == AvFILLp(av)+1);

The first causes a compiler warning on non-DEBUGGING builds about the empty if() body.

The code also introduces an unused variable warning for items on non-DEBUGGING builds.

Tony

---
via perlbug:  queue: perl5 status: open
https://rt.perl.org/Ticket/Display.html?id=120939

• [perl #120939] [PATCH] refactor and fix leak in const_av_xsub by Tony Cook via RT