Everything

php

[RFC] Internal operator overloading and GMP improvements

php.internals

Nikita Popov

13 years ago

Hi internals!

https://wiki.php.net/rfc/operator_overloading_gmp

This RFC proposes to add operator overloading for INTERNAL classes.
Furthermore it exemplarily implements the new API for GMP.

Thanks,
Nikita

Antony Dovgal

13 years ago

On 2013-05-12 19:25, Nikita Popov wrote:

> Hi internals!
>
> https://wiki.php.net/rfc/operator_overloading_gmp
>
> This RFC proposes to add operator overloading for INTERNAL classes.
> Furthermore it exemplarily implements the new API for GMP.

IMO the proposal B is quite reasonable change, but the proposal A (i.e. the operator overloading part) is definitely an overkill.
A simple benchmark should demonstrate that using GMP for basic arithmetic would kill performance in quite a brutal way.

--
Wbr,
Antony Dovgal
---
http://pinba.org - realtime profiling for PHP

Pierre Joye

13 years ago

On Sun, May 12, 2013 at 7:50 PM, Antony Dovgal <tony@daylessday.org> wrote:

> On 2013-05-12 19:25, Nikita Popov wrote:
>>
>> Hi internals!
>>
>> https://wiki.php.net/rfc/operator_overloading_gmp
>>
>> This RFC proposes to add operator overloading for INTERNAL classes.
>> Furthermore it exemplarily implements the new API for GMP.
>
>
> IMO the proposal B is quite reasonable change, but the proposal A (i.e. the
> operator overloading part) is definitely an overkill.
> A simple benchmark should demonstrate that using GMP for basic arithmetic
> would kill performance in quite a brutal way.

Right, the difficulty here is to use it for large numbers only. It
reduces the impact for standard ranges but do not totally suppress it.

Cheers,

--
Pierre

@pierrejoye |  http://www.libgd.org

Nikita Popov

13 years ago

On Sun, May 12, 2013 at 7:50 PM, Antony Dovgal <tony@daylessday.org> wrote:

> On 2013-05-12 19:25, Nikita Popov wrote:
>
>> Hi internals!
>>
>> https://wiki.php.net/rfc/**operator_overloading_gmp<https://wiki.php.net/rfc/operator_overloading_gmp>
>>
>> This RFC proposes to add operator overloading for INTERNAL classes.
>> Furthermore it exemplarily implements the new API for GMP.
>>
>
> IMO the proposal B is quite reasonable change, but the proposal A (i.e.
> the operator overloading part) is definitely an overkill.
> A simple benchmark should demonstrate that using GMP for basic arithmetic
> would kill performance in quite a brutal way.
>

I think this is a misunderstanding. I do not suggest to use GMP for all
arithmetic and also do not suggest to auto-promote to GMP for large numbers.

The operator overloading only comes into play if one of the operands is
already a GMP instance.

Regarding performance: The addition of the operator overloading does have a
measurable impact on performance. The switch of GMP from resources to
objects also does not show any clear change either way. What does become
faster is if a gmp function gets a non-gmp argument and needs to cast it to
GMP (this is faster because now only the mpz_t instance is created and not
a full resource). Obviously when the overloaded operators are used rather
than the functions it's faster too.

Nikita

Nikita Popov

13 years ago

On Sun, May 12, 2013 at 9:02 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> I think this is a misunderstanding. I do not suggest to use GMP for all
> arithmetic and also do not suggest to auto-promote to GMP for large numbers.
>
> The operator overloading only comes into play if one of the operands is
> already a GMP instance.
>
> Regarding performance: The addition of the operator overloading does have
> a measurable impact on performance.
>

Ooops, that was a typo. I wanted to say "does NOT have a measurable impact
on performance".

> The switch of GMP from resources to objects also does not show any clear
> change either way. What does become faster is if a gmp function gets a
> non-gmp argument and needs to cast it to GMP (this is faster because now
> only the mpz_t instance is created and not a full resource). Obviously when
> the overloaded operators are used rather than the functions it's faster too.
>

I tweaked the implementation a bit and now it seems to be faster in any
case. I added some numbers in
https://wiki.php.net/rfc/operator_overloading_gmp#performance

Nikita

Sara Golemon

13 years ago

Are we ignoring the ZEND_IS_SMALLER issue?

if ($gmp > 123) { ... }

There's no ZEND_IS_GREATER opcode, so it gets quietly turned into:

if (123 < $gmp) { ... }

Which will be confusing.

I dealt with this in operator by having the user apply a patch before
building:
https://github.com/php/pecl-php-operator/blob/master/compare-greater-5.1.2.diff

If we're going to bake this into ZE, then we should just add
ZEND_IS_GREATER(_OR_EQUAL) opcode.

-Sara

On Sun, May 12, 2013 at 12:02 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Nikita Popov

13 years ago

On Sun, May 12, 2013 at 9:57 PM, Sara Golemon <pollita@php.net> wrote:

> Are we ignoring the ZEND_IS_SMALLER issue?
>

Not ignoring it :) It's mentioned in one sentence: "The operators >, >=, [...]
are indirectly supported by the following compiler transformations: [...]"

if ($gmp > 123) { ... }

>
> There's no ZEND_IS_GREATER opcode, so it gets quietly turned into:
>
> if (123 < $gmp) { ... }
>
> Which will be confusing.
>

Why would this be confusing? I'd agree if this happened in userland (people
could wonder why the operators are swapped), but internally we are already
dealing with this anyway. E.g. when you implement compare_objects you have
to be aware of this (to understand stuff like the return 1 trick).

> I dealt with this in operator by having the user apply a patch before
> building:
> https://github.com/php/pecl-php-operator/blob/master/compare-greater-5.1.2.diff
>
> If we're going to bake this into ZE, then we should just add
> ZEND_IS_GREATER(_OR_EQUAL) opcode.
>

Not sure this is really necessary, but I have no problem with doing that
either.

Nikita

Stas Malyshev

13 years ago

Hi!

> Why would this be confusing? I'd agree if this happened in userland (people
> could wonder why the operators are swapped), but internally we are already
> dealing with this anyway. E.g. when you implement compare_objects you have
> to be aware of this (to understand stuff like the return 1 trick).

Your code suggests (even though RFC never says it) that the left operand
defines the comparison. However, for the switched operations, the right
operand would then define the comparison. It is pretty confusing, IMO.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Sara Golemon

13 years ago

>
> > Why would this be confusing? I'd agree if this happened in userland
> (people
> > could wonder why the operators are swapped), but internally we are
> already
> > dealing with this anyway. E.g. when you implement compare_objects you
> have
> > to be aware of this (to understand stuff like the return 1 trick).
>
> Your code suggests (even though RFC never says it) that the left operand
> defines the comparison. However, for the switched operations, the right
> operand would then define the comparison. It is pretty confusing, IMO.
>
> This.  I see in zend_object_do_operation that op1 (LHS) has priority, but

if it's not an object, then op2(RHS) gets the chance to handle it.  So it
works fine in the simple case of (Object and Non-Object), but if you have
two different Objects, both implementing operator overloading in
potentially different ways then the precedence order matters more.  By
having a separate opcode for GREATER, the user can explicitly state who
they want to get precedence.

I realize I'm potentially trying to solve a problem which doesn't exist,
but separating out smaller/greater is a fairly trivial change, so I'd
rather we did it now and avoid potential fail.

-Sara

Dmitry Stogov

13 years ago

Hi Nikita,

The patch looks quite good.
However, it must slow down each comparison operator (even if it compares
two integers).

I would suggest overloading of CMP operator instead of separate <, <=, ==,
!=, >, >=.

Also it may make sense to think about overloading of unary operators to
provide a solid decision.

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.

Thanks. Dmitry.


On Sun, May 12, 2013 at 7:25 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Nikita Popov

13 years ago

On Mon, May 13, 2013 at 7:49 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi Nikita,
>
> The patch looks quite good.
> However, it must slow down each comparison operator (even if it compares
> two integers).
>

In most cases it shouldn't, as the comparisons usually go through the
fast_is_*_functions, which have special handling for integers and doubles.
is_equal_function itself for example is only used in the implementation of
ZEND_CASE (why? shouldn't we use the fast_ one here too?) and stuff like
array_search.

I would suggest overloading of CMP operator instead of separate <, <=, ==,

> !=, >, >=.
>

But yeah, that sounds like a better solution. The advantages I see:
a) It will automatically work with sorting functions (and other stuff using
compare_function). This is a pretty big plus.
b) You don't have to implement the same (or similar) code for four (or six)
operators.
c) It (at least partially) also solves the concerns raised by Sara.

Would need an additional object handler though (as compare_objects works
only on objects, so it's currently not possible to support something like
$gmp == 0).

Nikita

Dmitry Stogov

13 years ago

Hi,

On Mon, May 13, 2013 at 1:24 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Mon, May 13, 2013 at 7:49 AM, Dmitry Stogov <dmitry@zend.com> wrote:
>
>> Hi Nikita,
>>
>> The patch looks quite good.
>> However, it must slow down each comparison operator (even if it compares
>> two integers).
>>
> In most cases it shouldn't, as the comparisons usually go through the
> fast_is_*_functions, which have special handling for integers and doubles.
> is_equal_function itself for example is only used in the implementation of
> ZEND_CASE (why? shouldn't we use the fast_ one here too?) and stuff like
> array_search.
>

Ahh, you are right :)

>
> I would suggest overloading of CMP operator instead of separate <, <=, ==,
>> !=, >, >=.
>>
>

> But yeah, that sounds like a better solution. The advantages I see:
> a) It will automatically work with sorting functions (and other stuff
> using compare_function). This is a pretty big plus.
> b) You don't have to implement the same (or similar) code for four (or
> six) operators.
> c) It (at least partially) also solves the concerns raised by Sara.
>

I'm glad, you are agree :)

> Would need an additional object handler though (as compare_objects works
> only on objects, so it's currently not possible to support something like
> $gmp == 0).
>
>

Or may be we may introduce additional opcode (or even pseudo-opcode)
ZEND_CMP to do it in the same way.
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.

Thanks. Dmitry.

Nikita Popov

13 years ago

On Mon, May 13, 2013 at 12:25 PM, Dmitry Stogov <dmitry@zend.com> wrote:

> Would need an additional object handler though (as compare_objects works
>> only on objects, so it's currently not possible to support something like
>> $gmp == 0).
>>
>
> Or may be we may introduce additional opcode (or even pseudo-opcode)
> ZEND_CMP to do it in the same way.
>

I implemented it with a separate handler for now (
https://github.com/nikic/php-src/commit/208442f84afd7ccd8e2dce8138c0950719a2e031),
but I'm also okay with moving it into do_operation. Not sure if it's a good
idea to add pseudo opcodes though.

Nikita

Dmitry Stogov

13 years ago

Hi Nikita,

I didn't get why do we need separate zend_std_compare() function.
May be I just didn't look careful :)

It would be great to look into the patch between master and current of your
branch.
It would be more clear than internal patches.

Thanks. Dmitry.


On Mon, May 13, 2013 at 2:50 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Nikita Popov

13 years ago

On Mon, May 13, 2013 at 1:09 PM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi Nikita,
>
> I didn't get why do we need separate zend_std_compare() function.
> May be I just didn't look careful :)
>

Good point, that was not really necessary. I moved the code back into
compare_function.

> It would be great to look into the patch between master and current of
> your branch.
>

You can find a diff between master and my branch on the PR:
https://github.com/php/php-src/pull/342/files
The relevant diff for the compare handler is here:
https://github.com/php/php-src/pull/342/files#L2R1581

Nikita

Dmitry Stogov

13 years ago

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)

- Increment and decrement operators in PHP may have different semantic than
+=1, but I it's probably OK to use ADD/SUB for them.

- 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...

I didn't look into GMP part.

Thanks. Dmitry.


On Mon, May 13, 2013 at 8:16 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Nikita Popov

13 years ago

On Tue, May 14, 2013 at 7:43 AM, Dmitry Stogov <dmitry@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

Dmitry Stogov

13 years ago

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.ppv@gmail.com> wrote: