Skip to content

Improve performance of user-callbacked sort functions #18166

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

Merged
merged 1 commit into from
Mar 29, 2025

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Mar 27, 2025

There are 3 improvements that contribute to the performance:

  • We don't need to refcount the parameters because zend_call_function already does this when copying to the call frame.
  • We can avoid zval destruction logic in the common case that the zval is an IS_LONG.
  • We don't need to check for zend_call_function's return value nor if the retval zval became UNDEF, because that would return 0 while php_get_long also does that. We don't need to optimize for the exceptional case.

For this test script:

<?php
function cmp($a, $b)
{
    if ($a == $b) {
        return 0;
    }
    return ($a < $b) ? -1 : 1;
}

$a = range(0,500);
array_push($a, ...range(500,0));

for($i=0;$i<1000;$i++)
	usort($a, "cmp");

On an i7-4790:

Benchmark 1: ./sapi/cli/php comp.php
  Time (mean ± σ):     222.0 ms ±   4.9 ms    [User: 218.5 ms, System: 2.5 ms]
  Range (min … max):   216.1 ms … 234.8 ms    13 runs
 
Benchmark 2: ./sapi/cli/php_old comp.php
  Time (mean ± σ):     264.2 ms ±   7.7 ms    [User: 258.6 ms, System: 1.9 ms]
  Range (min … max):   256.9 ms … 284.2 ms    10 runs
 
Summary
  ./sapi/cli/php comp.php ran
    1.19 ± 0.04 times faster than ./sapi/cli/php_old comp.php

On an i7-1185G7:

Benchmark 1: ./sapi/cli/php comp.php
  Time (mean ± σ):     147.0 ms ±   2.5 ms    [User: 144.2 ms, System: 2.8 ms]
  Range (min … max):   144.7 ms … 154.6 ms    19 runs
 
Benchmark 2: ./sapi/cli/php_old comp.php
  Time (mean ± σ):     184.3 ms ±   1.6 ms    [User: 180.7 ms, System: 3.3 ms]
  Range (min … max):   182.7 ms … 188.9 ms    16 runs
 
Summary
  ./sapi/cli/php comp.php ran
    1.25 ± 0.02 times faster than ./sapi/cli/php_old comp.php

Note that not only usort improves but it's the easiest to show.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is still wip, but the current code looks correct so far.

}
zend_call_function(&BG(user_compare_fci), &BG(user_compare_fci_cache));
zend_long ret = php_get_long(&retval);
return ZEND_NORMALIZE_BOOL(ret);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has nothing to do with your code, but: What a weird macro name. 😅

@nielsdos nielsdos marked this pull request as ready for review March 28, 2025 18:05
@nielsdos nielsdos requested a review from bukka as a code owner March 28, 2025 18:05
@nielsdos
Copy link
Member Author

I'm going to leave it at this for now. Improving further is possible by handling the deprecation logic slightly smarter but that only gives a few milliseconds improvement and does not seem worth it given that the code will be gone in the near future anyway.
So no changes since the review except for me adding a description to the PR with benchmarks and an explanation.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nielsdos nielsdos merged commit 3142193 into php:master Mar 29, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants