zend_compile: Optimize `sprintf()` into a rope #14546

TimWolla · 2024-06-12T12:22:52Z

This optimization will compile sprintf() using only %s placeholders into a rope at compile time, effectively making those calls equivalent to the use of string interpolation, with the added benefit of supporting arbitrary expressions instead of just expressions starting with a $.

For a synthetic test using:

<?php

$a = 'foo';
$b = 'bar';

for ($i = 0; $i < 100_000_000; $i++) {
    sprintf("%s-%s", $a, $b);
}

This optimization yields a 2.1× performance improvement:

$ hyperfine 'sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php' \
      '/tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php'
Benchmark 1: sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php
  Time (mean ± σ):      1.869 s ±  0.033 s    [User: 1.865 s, System: 0.003 s]
  Range (min … max):    1.840 s …  1.945 s    10 runs

Benchmark 2: /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php
  Time (mean ± σ):      4.011 s ±  0.034 s    [User: 4.006 s, System: 0.005 s]
  Range (min … max):    3.964 s …  4.079 s    10 runs

Summary
  sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php ran
    2.15 ± 0.04 times faster than /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php

This optimization comes with a small and probably insignificant behavioral change: If one of the values cannot be (cleanly) converted to a string, for example when attempting to insert an object that is not Stringable, the resulting Exception will naturally not show the sprintf() call in the resulting stack trace, because there is no call to sprintf().

Nevertheless it will correctly point out the line of the sprintf() call as the source of the Exception, pointing the user towards the correct location.

I'd like to thank Ilija for his preliminary review and great feedback during development of this 😃

This just extracts the implementation as-is into a dedicated function to make it reusable in preparation of a future commit.

…lize()`

… a single constant string Without this Opcache will trigger a use-after-free in `zend_optimizer_compact_literals()`. Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>

This optimization will compile `sprintf()` using only `%s` placeholders into a rope at compile time, effectively making those calls equivalent to the use of string interpolation, with the added benefit of supporting arbitrary expressions instead of just expressions starting with a `$`. For a synthetic test using: <?php $a = 'foo'; $b = 'bar'; for ($i = 0; $i < 100_000_000; $i++) { sprintf("%s-%s", $a, $b); } This optimization yields a 2.1× performance improvement: $ hyperfine 'sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php' \ '/tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php' Benchmark 1: sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php Time (mean ± σ): 1.869 s ± 0.033 s [User: 1.865 s, System: 0.003 s] Range (min … max): 1.840 s … 1.945 s 10 runs Benchmark 2: /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php Time (mean ± σ): 4.011 s ± 0.034 s [User: 4.006 s, System: 0.005 s] Range (min … max): 3.964 s … 4.079 s 10 runs Summary sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php ran 2.15 ± 0.04 times faster than /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php This optimization comes with a small and probably insignificant behavioral change: If one of the values cannot be (cleanly) converted to a string, for example when attempting to insert an object that is not `Stringable`, the resulting Exception will naturally not show the `sprintf()` call in the resulting stack trace, because there is no call to `sprintf()`. Nevertheless it will correctly point out the line of the `sprintf()` call as the source of the Exception, pointing the user towards the correct location.

iluuu1994

Nice! Didn't find any mistakes. Maybe request a review from Dmitry too.

Zend/zend_compile.c

iluuu1994 · 2024-06-12T13:58:14Z

Zend/zend_compile.c

+			if (rope_elements == 0) {
+				rope_init_lineno = get_next_op_number();
+			}
+			opline = zend_compile_rope_add(result, rope_elements++, &const_node);


Nit: For the case of %%, the merging of constant strings would actually be beneficial within zend_compile_rope_add(). Otherwise we're compiling sprintf('%%foo') this to '%' . 'foo', essentially. Although the optimizer probably takes care of this.

I expect %% to be fairly rare within format strings. The previous remark about “clarity” also applies here: Merging of consecutive constants should ideally happen in a generic fashion, instead of requiring every user to reimplement it.

Right. zend_compile_encaps_list() essentially does the same thing, which is what I was referring to. Hence, moving it to zend_compile_rope_add() would make it usable for both. That said, I wasn't expecting you to do that, but just noted it as a possible follow-up, and/or a potential // TODO to be added.

Zend/zend_compile.c

…mization

TimWolla · 2024-06-12T14:28:20Z

Regarding the “initial version” remark: Once this is reviewed and merged, I hope to also add support for plain %d placeholders, which I expect to be the second most common type of placeholder. I expect that to just require an additional (int) cast, given that %d just prints integers as-is.

“Named placeholders” (i.e. %1$s) might also be possible to support as a follow-up, given that they do not contain any additional logic either.

Girgias

I can't comment about the rope stuff, as I frankly don't understand it, but this looks great as a performance improvement!

Zend/zend_compile.c

Girgias · 2024-06-13T01:45:41Z

Zend/zend_compile.c

+			if (Z_TYPE(elements[i].u.constant) != IS_ARRAY) {
+				convert_to_string(&elements[i].u.constant);
+			}


You could potentially check if it is an object if it implements the Stringable interface? It would miss oddities like GMP not implementing the interface, although it can be cast to a string (which we really should fix...) but should work except if the __toString() method throws an error.

EDIT: I realized the issue is that one would need to possibly trigger autoloading to check the class....

These are compile time const expressions, they cannot currently contain objects (not even enums).

…c_sprintf()

mvorisek · 2024-06-13T07:41:13Z

Zend/zend_compile.c

@@ -4712,6 +4712,171 @@ static void zend_compile_ns_call(znode *result, znode *name_node, zend_ast *args
 }
 /* }}} */

+static zend_op *zend_compile_rope_add(znode *result, uint32_t num, znode *elem_node);


The benchmark shows decreased performance:

Please explain how this change would be able to affect bench.php.

Please also explain how the addition of two comments to the source code significantly changed the results compared to these: https://github.com/php/php-src/actions/runs/9484406283

maybe the performance degradation is comming from #14054 and the base branch used is detected wrongly?

dstogov

I don't see any problems

alexandre-daubois · 2024-06-13T10:01:26Z

Hi! This fail just popped in Symfony CI: https://github.com/symfony/symfony/actions/runs/9497428991/job/26173977979 (detected in symfony/symfony#57379, for the record). It seems this PR triggered it (older CI runs don't have the failure).

1) Symfony\Component\VarDumper\Tests\Caster\FFICasterTest::testCastNonTrailingCharPointer
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 FFI\CData<char*> size 8 align 8 {
-  cdata: "Hello World!%s"
+  cdata: b"Hello World!\x011RK`\x1EÐþ\x16\x7F\x00"
 }

/home/runner/work/symfony/symfony/src/Symfony/Component/VarDumper/Test/VarDumperTestTrait.php:52
/home/runner/work/symfony/symfony/src/Symfony/Component/VarDumper/Tests/Caster/FFICasterTest.php:219

Could this be considered a BC break? It's just this test that's causing the problem, maybe it's a fix to be made on the Symfony side. Just thought I'd give you a heads-up in case it alerts you to something obvious. 🙂

Direct link to the test case, if it helps: https://github.com/symfony/symfony/blob/678abb4b128c0bd9f6db83a280bd5355ce234aec/src/Symfony/Component/VarDumper/Tests/Caster/FFICasterTest.php#L205

iluuu1994 · 2024-06-13T10:03:35Z

@alexandre-daubois Thanks for the report! This shouldn't change behavior, so the issue is legit. @TimWolla Please have a look when you have time.

TimWolla · 2024-06-13T10:45:46Z

@alexandre-daubois Thank you for the report. I've taken a look and it appears the issue is unrelated. It also happens when I disable the optimization by inserting return FAILURE; at the top of the zend_compile_func_sprintf() function. I am also not seeing any calls to sprintf() nearby the test (assertDumpMatchesFormat() does not use it).

Note that I am only able to reproduce the issue with a production build, a debug build of PHP (--enable-debug) does not reproduce it.

TimWolla · 2024-06-13T10:50:27Z

It's also visible in the CI log that the PHP 8.4 build does not yet include this PR:

✓ PHP Installed PHP 8.4.0-dev (18cfd94)

18cfd94 is the commit right before the merge of this PR.

alexandre-daubois · 2024-06-13T10:59:24Z

Oh right, that's weird. I'll have a deeper look at the test case, thank you for having a look at it!

TimWolla · 2024-06-13T11:05:57Z

25360ef is the first bad commit

commit 25360ef24951f1c6b83f8bf85fbdcaff4a1a40e1
Author: Arnaud Le Blanc <arnaud.lb@gmail.com>
Date:   Wed Jun 12 14:02:48 2024 +0200

    Detect heap freelist corruption (#14054)
    
    We keep track of free slots by organizing them in a linked list, with the
    first word of every free slot being a pointer to the next one.
    
    In order to make corruptions more difficult to exploit, we check the consistency
    of these pointers before dereference by comparing them with a shadow. The shadow
    is a copy of the pointer, stored at the end of the slot.
    
    Before this change, an off-by-1 write is enough to produce a valid freelist
    pointer. After this change, a bigger out of bound write is required for that.
    The difficulty is increase further by mangling the shadow with a secret, and
    byte-swapping it, which increases the minimal required out of bound write
    length.
    
    Closes GH-14054

 Zend/zend.c       |   3 +-
 Zend/zend_alloc.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 199 insertions(+), 17 deletions(-)

git bisect start
# status: waiting for both good and bad commits
# bad: [1e7aac315ef12a0170351b8c7f406077b51976df] zend_compile: Optimize `sprintf()` into a rope (#14546)
git bisect bad 1e7aac315ef12a0170351b8c7f406077b51976df
# status: waiting for good commit(s), bad commit known
# good: [da7bc2ea046725e901f5eb177c57bdac1b38430b] Merge branch 'PHP-8.3'
git bisect good da7bc2ea046725e901f5eb177c57bdac1b38430b
# good: [61a0e3bd19f5f6524561e82236823a44c306ea6b] Sync HAVE_OPENSSL* symbols (#14333)
git bisect good 61a0e3bd19f5f6524561e82236823a44c306ea6b
# good: [d545b1d64350cac9cbf27859ad44d3ba32f6b736] Add missing ext/pcre dependency for ext/pgsql (#14541)
git bisect good d545b1d64350cac9cbf27859ad44d3ba32f6b736
# bad: [25360ef24951f1c6b83f8bf85fbdcaff4a1a40e1] Detect heap freelist corruption (#14054)
git bisect bad 25360ef24951f1c6b83f8bf85fbdcaff4a1a40e1
# good: [d1048a0869ea0505db252daaeb3fec5eab027f1c] Add zend_random_bytes(), zend_random_bytes_insecure() functions (#14054)
git bisect good d1048a0869ea0505db252daaeb3fec5eab027f1c
# first bad commit: [25360ef24951f1c6b83f8bf85fbdcaff4a1a40e1] Detect heap freelist corruption (#14054)

/cc @arnaud-lb

arnaud-lb · 2024-06-13T12:34:06Z

I believe it's not a bug in php-src. I've reported a symfony issue: symfony/symfony#57387.

Thank you @alexandre-daubois @TimWolla

mvorisek · 2024-06-18T12:10:40Z

@TimWolla I have two questions about this PR:

can vsprintf be optimized as well for consistency?
why is sprintf not marked as CTE [1] (compile time evaluable)?

[1] https://github.com/php/php-src/blob/e4a8d5b16f/ext/standard/basic_functions.stub.php#L2999

iluuu1994 · 2024-06-18T12:27:46Z

@mvorisek Because they are locale-dependent for floating points.

TimWolla · 2024-06-18T12:54:03Z

can vsprintf be optimized as well for consistency?

From my experience vsprintf() is used much more rarely compared to sprintf() and if it is used then the format string is not a string literal. Combining a format string literal with an array parameter seems like an odd choice, because you might as well pass the parameters separately.

For that reason I don't plan to do that, but feel free to send a PR if you have data that indicates it would be worth it.

Girgias · 2024-06-19T01:39:07Z

@mvorisek Because they are locale-dependent for floating points.

That's incorrect since 8.0 with the Locale-independent float to string cast RFC getting accepted.
However, floating points still depend on the 'precision' INI setting: https://3v4l.org/mMjuK

TimWolla and others added 4 commits June 12, 2024 10:08

zend_compile: Add zend_compile_rope_finalize()

3d0f581

This just extracts the implementation as-is into a dedicated function to make it reusable in preparation of a future commit.

zend_compile: Use clearer parameter names for `zend_compile_rope_fina…

72f9b24

…lize()`

zend_compile: Fix zend_compile_rope_finalize() for ropes containing…

ccc1ac8

… a single constant string Without this Opcache will trigger a use-after-free in `zend_optimizer_compact_literals()`. Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>

TimWolla requested a review from iluuu1994 June 12, 2024 12:22

TimWolla requested a review from bukka as a code owner June 12, 2024 12:22

github-actions bot added Category: Engine Extension: standard labels Jun 12, 2024

iluuu1994 approved these changes Jun 12, 2024

View reviewed changes

zend_compile: Eagerly handle empty format strings in sprintf() opti…

cd788a2

…mization

TimWolla requested a review from dstogov June 12, 2024 14:25

Girgias reviewed Jun 13, 2024

View reviewed changes

TimWolla added 2 commits June 13, 2024 08:55

zend_compile: Add additional explanatory comments to zend_compile_fun…

490f200

…c_sprintf()

Add zero-argument test to sprintf_rope_optimization_001.phpt

f05206d

mvorisek reviewed Jun 13, 2024

View reviewed changes

dstogov approved these changes Jun 13, 2024

View reviewed changes

TimWolla merged commit 1e7aac3 into php:master Jun 13, 2024
11 checks passed

TimWolla deleted the sprintf-optimize branch June 13, 2024 08:41

TimWolla mentioned this pull request Jun 13, 2024

zend_compile: Add support for %d to sprintf() optimization #14561

Merged

arnaud-lb mentioned this pull request Jun 13, 2024

FFICasterTest::testCastNonTrailingCharPointer relies on undefined behaviors symfony/symfony#57387

Closed

sebastianbergmann mentioned this pull request Jun 17, 2024

Remove unnecessary sprintf() in hot path sebastianbergmann/exporter#58

Merged

mvorisek mentioned this pull request Jun 18, 2024

feat: Add sprintf in the list of compiler optimized functions PHP-CS-Fixer/PHP-CS-Fixer#8092

Merged

arokettu mentioned this pull request Sep 25, 2024

sprintf() is now optimized by the compiler slevomat/coding-standard#1697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zend_compile: Optimize `sprintf()` into a rope #14546

zend_compile: Optimize `sprintf()` into a rope #14546

TimWolla commented Jun 12, 2024

iluuu1994 left a comment

iluuu1994 Jun 12, 2024

TimWolla Jun 12, 2024

iluuu1994 Jun 12, 2024

TimWolla commented Jun 12, 2024

Girgias left a comment

Girgias Jun 13, 2024 •

edited

Loading

iluuu1994 Jun 13, 2024

mvorisek Jun 13, 2024

TimWolla Jun 13, 2024

mvorisek Jun 13, 2024

dstogov left a comment

alexandre-daubois commented Jun 13, 2024 •

edited

Loading

iluuu1994 commented Jun 13, 2024

TimWolla commented Jun 13, 2024

TimWolla commented Jun 13, 2024 •

edited

Loading

alexandre-daubois commented Jun 13, 2024

TimWolla commented Jun 13, 2024

arnaud-lb commented Jun 13, 2024 •

edited

Loading

mvorisek commented Jun 18, 2024

iluuu1994 commented Jun 18, 2024

TimWolla commented Jun 18, 2024

Girgias commented Jun 19, 2024 •

edited

Loading

zend_compile: Optimize sprintf() into a rope #14546

zend_compile: Optimize sprintf() into a rope #14546

Conversation

TimWolla commented Jun 12, 2024

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Jun 12, 2024

Choose a reason for hiding this comment

TimWolla Jun 12, 2024

Choose a reason for hiding this comment

iluuu1994 Jun 12, 2024

Choose a reason for hiding this comment

TimWolla commented Jun 12, 2024

Girgias left a comment

Choose a reason for hiding this comment

Girgias Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

iluuu1994 Jun 13, 2024

Choose a reason for hiding this comment

mvorisek Jun 13, 2024

Choose a reason for hiding this comment

TimWolla Jun 13, 2024

Choose a reason for hiding this comment

mvorisek Jun 13, 2024

Choose a reason for hiding this comment

dstogov left a comment

Choose a reason for hiding this comment

alexandre-daubois commented Jun 13, 2024 • edited Loading

iluuu1994 commented Jun 13, 2024

TimWolla commented Jun 13, 2024

TimWolla commented Jun 13, 2024 • edited Loading

alexandre-daubois commented Jun 13, 2024

TimWolla commented Jun 13, 2024

arnaud-lb commented Jun 13, 2024 • edited Loading

mvorisek commented Jun 18, 2024

iluuu1994 commented Jun 18, 2024

TimWolla commented Jun 18, 2024

Girgias commented Jun 19, 2024 • edited Loading

zend_compile: Optimize `sprintf()` into a rope #14546

zend_compile: Optimize `sprintf()` into a rope #14546

Girgias Jun 13, 2024 •

edited

Loading

alexandre-daubois commented Jun 13, 2024 •

edited

Loading

TimWolla commented Jun 13, 2024 •

edited

Loading

arnaud-lb commented Jun 13, 2024 •

edited

Loading

Girgias commented Jun 19, 2024 •

edited

Loading