Hi Nikita,
Thanks for function renaming.
I'm agree about INC/DEC and unary operators (!, ~) implementation.
A single callback for all operators may not be always good, because classes
may overload not all but only few operators,
but technically it doesn't make any problems, so let keep it as is.
According to fast-path execution, I would really like to not introduce the
additional checks.
Especially for concat_function I would change it in the following way:
if (Z_TYPE_P(op1) != IS_STRING) {
if (Z_TYPE_P(op1) == IS_OBJECT && Z_OBJ_HANDLER_P(op1, do_operation))
{
return Z_OBJ_HANDLER_P(op1, do_operation)(ZEND_CONCAT, result, op1,
op2 TSRMLS_CC);
}
zend_make_printable_zval(op1, &op1_copy, &use_copy1);
}
if (Z_TYPE_P(op2) != IS_STRING) {
if (Z_TYPE_P(op2) == IS_OBJECT && Z_OBJ_HANDLER_P(op2, do_operation))
{
return Z_OBJ_HANDLER_P(op2, do_operation)(ZEND_CONCAT, result, op1,
op2 TSRMLS_CC);
} zend_make_printable_zval(op2, &op2_copy, &use_copy2);
}
And in similar way for mod, shift, etc
if (UNEXPECTED(Z_TYPE_P(op1) != IS_LONG)) {
if (Z_TYPE_P(op1) == IS_OBJECT && Z_OBJ_HANDLER_P(op1, do_operation))
{
return Z_OBJ_HANDLER_P(op1, do_operation)(ZEND_MOD, result, op1, op2
TSRMLS_CC);
}
zendi_convert_to_long(op1, op1_copy, result);
}
op1_lval = Z_LVAL_P(op1);
if (UNEXPECTED(Z_TYPE_P(op2) != IS_LONG)) {
if (Z_TYPE_P(op2) == IS_OBJECT && Z_OBJ_HANDLER_P(op2, do_operation))
{
return Z_OBJ_HANDLER_P(op2, do_operation)(ZEND_MOD, result, op1, op2
TSRMLS_CC);
} zendi_convert_to_long(op2, op2_copy, result);
}
I think this is the last technical issue :)
Thanks. Dmitry.
On Wed, May 15, 2013 at 1:18 AM, Nikita Popov <nikita....@gmail.com> wrote:
> On Tue, May 14, 2013 at 7:43 AM, Dmitry Stogov <dmi...@zend.com> wrote:
>
>> Hi Nikita,
>>
>> Few final notes:
>>
>> - I wouldn't change zend_object_compare_t into
>> zend_object_compare_objects_t. It would be better to name the new function
>> as zend_object_compare_zvals_t. (It's just for better backward
>> compatibility)
>>
> Done. Should I also call the handler itself compare_zvals then or can that
> stay as just compare?
>
> - Increment and decrement operators in PHP may have different semantic
>> than +=1, but I it's probably OK to use ADD/SUB for them.
>>
> I think we should just introduce this once a use case comes up.
>
>
>> - In some cases you insert call to zend_object_do_operation into the most
>> probable path (e.g. in mod_function). This would cause at lease 2
>> additional comparisons and may be conditional jumps. I think it would be
>> better to check for most probable operand types first...
>>
> For $a % $b the most common cases are already handled by
> fast_mod_function. mod_function is only directly called when doing $a %=
> $b. But in any case, I'm not sure how I could test the more probable cases
> first, as the next thing the code does is call convert_to_long. Of course I
> could copy the code from fast_mod_function in there, but that doesn't
> sounds like a good idea.
>
> I think the only operation that could really be a performance concern is
> concat_function, because that's a common operation without a fast_ variant.
> But even in that case I could not measure a difference in runtime (taking
> averages on 100M concatenations).
>
>
>> Also it may be better to use a table of callbacks for each overloaded
>> operand instead of single one that need to do switch anyway.
>>
> What would be the benefits? Better performance? Imho using a switch is
> handier when implementing the operators, because it requires less
> boilerplate code (no need to repeat function signature, variables, shared
> code etc).
>
>
>> Also it may make sense to think about overloading of unary operators to
>> provide a solid decision.
>>
> You mean being able to overload unary + and - directly rather than the
> current 0+$a / 0-$a transformation? I can see that this might be useful,
> but not sure it's worth it (would have to introduce new opcodes for that).
> I'd do the same as with the increment/decrement operators here: Only
> implement them once there is a specific use case.
>
> In case you think about user-level operator overloading in the future
>> (that may make sense :) it would be better to design them all together.
>>
> I was thinking about that too, but from the previous discussions on the
> topic I figured that there is zero chance for having that in PHP :/
>
> Thanks for the feedback!
> Nikita
>
