Skip to content

improve max() performance #11192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
staabm opened this issue May 6, 2023 · 2 comments · Fixed by #11194
Closed

improve max() performance #11192

staabm opened this issue May 6, 2023 · 2 comments · Fixed by #11194

Comments

@staabm
Copy link
Contributor

staabm commented May 6, 2023

Description

In a recent performance investigation I identified max() as a perf bottleneck in sebastianbergmann/diff#118

@bwoebi chimmed in and suggested max() should be improved at the php-src level.
see his in-detail investigation at sebastianbergmann/diff#118 (comment)

The following code:

<?php
~/php-src-8.2 # time ./sapi/cli/php -r '$b = 1; $c = 2; for ($i = 0; $i < 430000000; ++$i) { $a = max($b, $c); }'

real	0m6.924s
user	0m6.917s
sys	0m0.004s
~/php-src-8.2 # time ./sapi/cli/php -r '$b = 1; $c = 2; for ($i = 0; $i < 430000000; ++$i) { if ($b < $c) { $a = $b; } else { $a = $c; } }'

real	0m2.523s
user	0m2.514s
sys	0m0.008s

max() should be faster

PHP Version

8.2.5

Operating System

macos

@nielsdos
Copy link
Member

nielsdos commented May 6, 2023

This should probably be done for min too.

I see that is_smaller_or_equal_function is used here, which internally calls zend_compare. I previously also noticed that zend_compare is quite slow for primitives, and on my fork I have a patch that splits that function into a fast path and a slow path. So I wonder how much that would help in this use-case. I'll put it on my todo list to check this.

@Girgias
Copy link
Member

Girgias commented May 6, 2023

FYI I've started working on this, and yes min() should also be done.

@Girgias Girgias self-assigned this May 6, 2023
@Girgias Girgias linked a pull request May 6, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants