Speed up fetching of class entries for self:: parent:: and static:: (Bob's idea)

9 years ago by Dmitry Stogov — view source

unread

Hi,

Please take a look: https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

Thanks. Dmitry.

9 years ago by Dmitry Stogov — view source

unread

What is the general impression from the patch?
Should we commit it (with minor fixes and/or additional optimisations)?
Bob, do you see performance improvement on your app?

Thanks. Dmitry.

Hi,

Please take a look: https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

Thanks. Dmitry.

9 years ago by Bob Weinand — view source

unread

Am 28.10.2015 um 05:20 schrieb Dmitry Stogov dmitry@zend.com:
What is the general impression from the patch?
Should we commit it (with minor fixes and/or additional optimisations)?
Bob, do you see performance improvement on your app?

Thanks. Dmitry.

Hi,

Please take a look: https://github.com/php/php-src/pull/1604 https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

Thanks. Dmitry.

In general I like the patch, if only for the reason that it splits up the monster zend_fetch_var_address_helper actually was into two helpers…

I benchmarked https://github.com/amphp/aerys/blob/master/lib/HPack.php with the following code:

// Two example request headers from RFC 7541
$data1 = hex2bin(str_replace(" ", "", "8286 8441 8cf1 e3c2 e5f2 3a6b a0ab 90f4 ff"));
$data2 = hex2bin(str_replace(" ", "", "8286 84be 5886 a8eb 1064 9cbf"));

for ($i = 0; $i < 1e5; $i++) {
$obj = new HPack;
$obj->decode($data1);
$obj->decode($data2);
}

and got 6.27 vs. 6.01 seconds (4%). With the one or other optimization (like mentioned in PR: not doing a zend_fetch_class() on each call, maybe more), it'll be probably even more.

(I realize there's a lot overhead, but that's because I try to have a not too synthetic benchmark; but at least these methods are the bottleneck of most HTTP/2 traffic in this server app, so, if I can speed these up, it will be significant.)

Bob

9 years ago by Xinchen Hui — view source

unread

Hey:

Am 28.10.2015 um 05:20 schrieb Dmitry Stogov dmitry@zend.com:

What is the general impression from the patch?
Should we commit it (with minor fixes and/or additional optimisations)?
Bob, do you see performance improvement on your app?

Thanks. Dmitry.

Hi,

Please take a look: https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

Thanks. Dmitry.

In general I like the patch, if only for the reason that it splits up the
monster zend_fetch_var_address_helper actually was into two helpers…

I benchmarked https://github.com/amphp/aerys/blob/master/lib/HPack.php with
the following code:

// Two example request headers from RFC 7541
$data1 = hex2bin(str_replace(" ", "", "8286 8441 8cf1 e3c2 e5f2 3a6b a0ab
90f4 ff"));
$data2 = hex2bin(str_replace(" ", "", "8286 84be 5886 a8eb 1064 9cbf"));

for ($i = 0; $i < 1e5; $i++) {
$obj = new HPack;
$obj->decode($data1);
$obj->decode($data2);
}

and got 6.27 vs. 6.01 seconds (4%). With the one or other optimization
(like mentioned in PR: not doing a zend_fetch_class() on each call, maybe
more), it'll be probably even more.

4% is also a significant improvement.

I 'd like to see it to be committed to master branch, we can keep do
something opts base on this after it merged.

thanks

(I realize there's a lot overhead, but that's because I try to have a not
too synthetic benchmark; but at least these methods are the bottleneck of
most HTTP/2 traffic in this server app, so, if I can speed these up, it
will be significant.)

Bob

--
Xinchen Hui
@Laruence
http://www.laruence.com/

9 years ago by Andrea Faulds — view source

unread

Hi Dmitry,

Dmitry Stogov wrote:

Please take a look: https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

10KB VM size increase for insignificant real-life improvement doesn't
sound good. Surely that would hurt performance on some systems?

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Dmitry Stogov — view source

unread

Hi Andrea,

Hi Dmitry,

Dmitry Stogov wrote:

Please take a look: https://github.com/php/php-src/pull/1604
The speed improvement on real-life is insignificant.
VM size is increased by 10KB.

I don't have a strong opinion, if this should go into master or not.

10KB VM size increase for insignificant real-life improvement doesn't
sound good. Surely that would hurt performance on some systems?

Of corse, this may affect performance in negative way.
This occurs, mainly not becuae differenf code size, but because of
different code layout.
The same change may affect performance in different ways on diferent
systems.
Anyway, this patch is not only for speed, but for clenup and making
difference, between regular variables and static properties.

We will try to reduce the VM code size later.

Thanks. Dmitry.

Thanks.

--
Andrea Faulds
http://ajf.me/