Hi Dmitry,
Le Monday 11 July 2011 09:49:27, Dmitry Stogov a écrit :
> Hi Arnaud,
>
> Is the main advantage of the patch - elimination of zval copy
> constructor for IS_VAR and IS_CV operands?
Yes.
Currently the ternary operator does a copy of its second and third operands.
This is costly when the operand is an array for example (the array has to be
copied, and all its elements addref'ed).
The patch avoids this when the operand is a variable (VAR/CV).
$b = range(1, 10000);
for ($i = 0; $i < 100000; ++$i) {
true ? $b : $c
}
This code runs in more than a minute currently, because $b is copied on each
iteration.
With the patch, it runs in less than a hundredth or a second.
>
> Does it improve speed only for VAR/CV array operands?
Yes.
For TMP operands this is already fast (no copy). For CONST operands there is
still a copy.
>
> Will the following script require an extra zval ealloc/efree with the
> patch?
>
> <?php
> function foo($x, $y) {
> $x ? $y : "default";
> }
>
> foo(false, "something");
> ?>
Yes, when only one of the operands is a TMP/CONST, this requires an extra zval
alloc when the TMP/CONST is returned.
This does not happen when both operands and TMP/CONST, or when no operand is
TMP/CONST.
I believe that the cost of this alloc is cancelled by the gains on the other
operand (assuming that all ternary operators with a VAR/CV and a CONST/TMP
operands in a program will evenly return the VAR/CV or the CONST/TMP).
>
> For now the patch still looks a bit messing to me. Also, I would
> probably prefer to use new opcodes ZEND_QM_ASSIGN_VAR and ZEND_JMP_SET_VAR.
Ok, I'll change the patch to use new opcodes instead
>
> I'll try to think a bit more about it later today.
Thanks
>
> Thanks. Dmitry.
>
> On 07/08/2011 06:39 PM, Arnaud Le Blanc wrote:
> > Hi,
> >
> > Thanks for your answer.
> >
> >> Probably it would be better to support both TMP/VAR result. Or even use
> >
> > different opcodes.
> >
> > Yes. I've changed the patch so that it still uses IS_TMP_VAR when both
> > operands are IS_TMP_VAR or IS_CONST.
> >
> > I've also added some benchmarks in micro_benchmark.php, and a test.phpt
> > that tests all combinations of (true|false) ? (const|tmp|var|cv) :
> > (const|tmp|var| cv) (and also for the ?: operator).
> >
> > The test found a memory leak in the current trunk in the following code:
> >
> > true ? $a : $b; the result of $b is marked UNUSED, but not the result of
> > $a. I've modified zend_do_free() to add a ZEND_FREE op after the whole
> > expression instead.
> >
> > Best regards,
> >
> > Arnaud
> >
> > Le Friday 8 July 2011 07:51:22, vous avez écrit :
> >> Hi Arnaud,
> >>
> >> I haven't test your patch yet, but I got the main idea.
> >> I'll need to carefully analyze it. Probably it would be better to
> >> support both TMP/VAR result. Or even use different opcodes.
> >>
> >> Thanks. Dmitry.
> >>
> >> On 07/07/2011 10:45 PM, Arnaud Le Blanc wrote:
> >>> Hi Dmitry,
> >>>
> >>> The ternary operator always deep-copies its second or third operand,
> >>> and it is very slow compared to an if/else when the operand is an
> >>> array for example.
> >>>
> >>> I tried to improve this by avoiding copies when possible. I ran the
> >>> test suite and seen no problem, but I'm not sure if this is correct or
> >>> not; could you please review the attached patch ?
> >>>
> >>> The following script shows the improvements:
> >>>
> >>> $a = range(1, 10000);
> >>> for($i = 0; $i< 100000; ++$i) {
> >>>
> >>> $a = true ? $a : $a;
> >>>
> >>> }
> >>>
> >>> Without the patch, this runs in more than a minute. With the patch,
> >>> this runs in less than 0.01 second.
> >>>
> >>> I copied most of the code from the ZEND_RETURN handler; I'm not sure
> >>> why IS_CONST are copied as well ? (can't we prevent the IS_CONST from
> >>> being modified by incrementing its refcount ?)
> >>>
> >>> Best regards,
> >>>
> >>> Arnaud
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
