Repository navigation
Optimize performances of str_pad() - #19272
Conversation
| @@ -5737,6 +5737,27 @@ PHP_FUNCTION(substr_count) | |||
| } | |||
| /* }}} */ | |||
|
|
|||
| static inline void php_str_pad_fill(char *dest, size_t pad_chars, const char *pad_str, size_t pad_str_len) { | |||
There was a problem hiding this comment.
| static inline void php_str_pad_fill(char *dest, size_t pad_chars, const char *pad_str, size_t pad_str_len) { | |
| static zend_always_inline void php_str_pad_fill(char *dest, size_t pad_chars, const char *pad_str, size_t pad_str_len) { |
Using zend_always_inline increases the likelihood that the function will actually be inlined.
Also, instead of passing char *dest and size_t pad_chars as separate arguments, how about accepting a zend_string and extracting the values within this inline function?
There was a problem hiding this comment.
Good idea! PR updated with both suggestions
fbfa654 to
24d2823
Compare
24d2823 to
ba1eb4c
Compare
TimWolla
left a comment
There was a problem hiding this comment.
I feel that this makes the API of php_str_pad_fill() a little safer to use, because it makes less assumptions about the target pointer.
With regard to the inlining: I generally trust the compiler to make better than decisions than I can do myself. I would make the function just static void without any inlining hints and let the compiler decide.
| for (i = 0; i < left_pad; i++) | ||
| ZSTR_VAL(result)[ZSTR_LEN(result)++] = pad_str[i % pad_str_len]; | ||
| if (left_pad > 0) { | ||
| php_str_pad_fill(result, left_pad, pad_str, pad_str_len); |
There was a problem hiding this comment.
| php_str_pad_fill(result, left_pad, pad_str, pad_str_len); | |
| php_str_pad_fill(ZSTR_VAL(result) + ZSTR_LEN(result), left_pad, pad_str, pad_str_len); |
There was a problem hiding this comment.
@SakiTakamachi advised otherwise if I get it right?
There was a problem hiding this comment.
It appears so, but the suggestion doesn't really make sense, since pad_chars cannot be piggy-backed on the zend_string. Perhaps Saki meant to pass pad_str and pad_str_len as a zend_string* (which would make sense to me).
There was a problem hiding this comment.
It makes the code a bit slower, going from ~48ms per run to ~58ms. Do we still want this?
There was a problem hiding this comment.
I think it’s fine to go with the faster one! :)
|
Please also see #19220 (comment) and #19220 (comment) for benchmarking advice. |
|
Thanks for the link, I'm having a look to hyperfine for this PR and #19276 |
ba1eb4c to
78dd005
Compare
|
PR description updated to use hyperfine |
The |
|
I think that my system-wide install right now is PHP 8.5 nightly. Just to be sure, I rerun against master and the branch: |
AWS x86_64 (c7i.24xl)
Laravel 11.1.2 demo app - 30 consecutive runs, 100 requests (sec)
Symfony 2.6.0 demo app - 30 consecutive runs, 100 requests (sec)
Wordpress 6.2 main page - 30 consecutive runs, 20 requests (sec)
bench.php - 25 consecutive runs (sec)
micro_bench.php - 25 consecutive runs (sec)
|
|
The above comment was due to test the real time benchmark :) Unfortunately, I guess most results should show zero difference, which is not currently the case :( The wordpress results are especially weird. I'll have to dig into it... cc. @iluuu1994 @arnaud-lb |
|
If I can be of any help, don't hesitate to tell me! It is indeed surprising |
AWS x86_64 (c7i.24xl)
Laravel 11.1.2 demo app - 30 consecutive runs, 100 requests (sec)
Symfony 2.6.0 demo app - 30 consecutive runs, 100 requests (sec)
Wordpress 6.2 main page - 30 consecutive runs, 20 requests (sec)
bench.php - 25 consecutive runs (sec)
micro_bench.php - 25 consecutive runs (sec)
|
|
@kocsismate FYI, I've written a small benchmarking helper for myself. https://github.com/iluuu1994/php-benchmark-diff It differs slightly from things like hyperfine in a few ways:
Not sure if this is useful to you, but wanted to mention it in case it is. It doesn't support JSON output yet. |
|
@iluuu1994 Nice thank you very much for the input! I've seen Tim's comment somewhere about hyperfine (and read a blog post about it by Volker), but didn't know how it exactly differs from what I do, but based on your description it looks very promising! Specifically, I'm pretty much interested about the p-value. I think my benchmark also discards some of the results, but I should look into it how I exactly implemented it. Alternating tests is also a very clever idea. I'll try to implement it at some point, because I'm still pretty much disappointed by my results :( Most tests used to be stable a few months ago, but they stopped being reliable a while ago. And curiously, microbenchmarks perform the worst. |
|
I see I kept the branch with the JSON output too. iluuu1994/php-benchmark-diff@master...json I planned to replace the Valgrind benchmark, but unfortunately collecting CPU cycles with |
|
Friendly ping, are you fine merging this? It's safer now that 8.5 has its branch |
iluuu1994
left a comment
There was a problem hiding this comment.
Looks correct to me. Thanks!
|
One small thing that might make sense before merging is benchmarking small paddings/string. I reckon these are pretty common, and might slightly regress with this implementation. Regardless, I don't object. |
|
Here are the results with tiny padding/strings: <?php
$iterations = 1000000;
for ($i = 0; $i < $iterations; $i++) {
str_pad('hello', 10, ' ', STR_PAD_RIGHT);
}
/*
Benchmark 1: ./sapi/cli/php.branch bench_small_right.php
Time (mean ± σ): 1.360 s ± 0.015 s [User: 1.314 s, System: 0.037 s]
Range (min … max): 1.330 s … 1.378 s 10 runs
Benchmark 2: ./sapi/cli/php.master bench_small_right.php
Time (mean ± σ): 1.383 s ± 0.008 s [User: 1.340 s, System: 0.034 s]
Range (min … max): 1.367 s … 1.399 s 10 runs
Summary
./sapi/cli/php.branch bench_small_right.php ran
1.02 ± 0.01 times faster than ./sapi/cli/php.master bench_small_right.php
*/<?php
$iterations = 1000000;
for ($i = 0; $i < $iterations; $i++) {
str_pad('hello', 10, ' ', STR_PAD_LEFT);
}
/*
Benchmark 1: ./sapi/cli/php.branch bench_small_left.php
Time (mean ± σ): 1.404 s ± 0.068 s [User: 1.316 s, System: 0.039 s]
Range (min … max): 1.347 s … 1.549 s 10 runs
Benchmark 2: ./sapi/cli/php.master bench_small_left.php
Time (mean ± σ): 1.422 s ± 0.068 s [User: 1.296 s, System: 0.068 s]
Range (min … max): 1.347 s … 1.586 s 10 runs
Summary
./sapi/cli/php.branch bench_small_left.php ran
1.01 ± 0.07 times faster than ./sapi/cli/php.master bench_small_left.php
*/<?php
$iterations = 1000000;
for ($i = 0; $i < $iterations; $i++) {
str_pad('hello', 10, ' ', STR_PAD_BOTH);
}
/*
Benchmark 1: ./sapi/cli/php.branch bench_small_both.php
Time (mean ± σ): 1.376 s ± 0.012 s [User: 1.326 s, System: 0.038 s]
Range (min … max): 1.355 s … 1.392 s 10 runs
Benchmark 2: ./sapi/cli/php.master bench_small_both.php
Time (mean ± σ): 1.361 s ± 0.030 s [User: 1.265 s, System: 0.064 s]
Range (min … max): 1.318 s … 1.413 s 10 runs
Summary
./sapi/cli/php.master bench_small_both.php ran
1.01 ± 0.02 times faster than ./sapi/cli/php.branch bench_small_both.php
*/<?php
$iterations = 1000000;
for ($i = 0; $i < $iterations; $i++) {
str_pad('x', 5, ' ', STR_PAD_RIGHT);
}
/*
Benchmark 1: ./sapi/cli/php.branch bench_tiny_1char.php
Time (mean ± σ): 1.378 s ± 0.027 s [User: 1.242 s, System: 0.079 s]
Range (min … max): 1.330 s … 1.420 s 10 runs
Benchmark 2: ./sapi/cli/php.master bench_tiny_1char.php
Time (mean ± σ): 1.333 s ± 0.009 s [User: 1.264 s, System: 0.056 s]
Range (min … max): 1.321 s … 1.350 s 10 runs
Summary
./sapi/cli/php.master bench_tiny_1char.php ran
1.03 ± 0.02 times faster than ./sapi/cli/php.branch bench_tiny_1char.php
*/<?php
$iterations = 1000000;
for ($i = 0; $i < $iterations; $i++) {
str_pad('foo', 15, '.-', STR_PAD_RIGHT);
}
/*
Benchmark 1: ./sapi/cli/php.branch bench_multichar_pad.php
Time (mean ± σ): 1.361 s ± 0.010 s [User: 1.292 s, System: 0.059 s]
Range (min … max): 1.340 s … 1.376 s 10 runs
Benchmark 2: ./sapi/cli/php.master bench_multichar_pad.php
Time (mean ± σ): 1.402 s ± 0.012 s [User: 1.330 s, System: 0.060 s]
Range (min … max): 1.382 s … 1.422 s 10 runs
Summary
./sapi/cli/php.branch bench_multichar_pad.php ran
1.03 ± 0.01 times faster than ./sapi/cli/php.master bench_multichar_pad.php
*/Are you fine with these results? |
78dd005 to
3821c83
Compare
|
Looks good, thanks! |
I would like to propose this optimization for
str_pad(). Here is the benchmark code:And the results with right padding:
Left padding results:
The idea is to avoid modulo operation in loops and copying char by char. Instead, this PR prefers the bulk copy approach.