Skip to content

Sprintf optimize #2

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
wants to merge 4 commits into from
Closed

Sprintf optimize #2

wants to merge 4 commits into from

Conversation

TimWolla
Copy link
Owner

@TimWolla TimWolla commented Jun 3, 2024

No description provided.

zend_op *init_opline = CG(active_op_array)->opcodes + rope_init_lineno;
zend_compile_rope_finalize(result, rope_elements, init_opline, opline);

zval_ptr_dtor_str(&format_string.u.constant);

Choose a reason for hiding this comment

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

This misses the optimizations from zend_compile_encaps_list(), which concat()s adjacent string constants. I'm not sure how important this is for sprintf.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fairly unimportant, I'd say: Using a placeholder to insert a constant string is not really the expected use case of sprintf().

FWIW: Opcache will automatically merge ropes that are completely constant:

<?php

var_dump(sprintf("%s: %s-%s", __FILE__, "foo", "bar"));
$ sapi/cli/php -d zend_extension=(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.opt_debug_level=0x10000 test.php

$_main:
     ; (lines=9, args=0, vars=0, tmps=5)
     ; (before optimizer)
     ; php-src/test.php:1-4
     ; return  [] RANGE[0..0]
0000 INIT_FCALL 1 96 string("var_dump")
0001 T1 = ROPE_INIT 5 string("php-src/test.php")
0002 T1 = ROPE_ADD 1 T1 string(": ")
0003 T1 = ROPE_ADD 2 T1 string("foo")
0004 T1 = ROPE_ADD 3 T1 string("-")
0005 T0 = ROPE_END 4 T1 string("bar")
0006 SEND_VAL T0 1
0007 DO_ICALL
0008 RETURN int(1)
LIVE RANGES:
     1: 0001 - 0005 (rope)
string(25) "php-src/test.php: foo-bar"
$ sapi/cli/php -d zend_extension=(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.opt_debug_level=0x20000 test.php

$_main:
     ; (lines=4, args=0, vars=0, tmps=0)
     ; (after optimizer)
     ; php-src/test.php:1-4
0000 INIT_FCALL 1 96 string("var_dump")
0001 SEND_VAL string("php-src/test.php: foo-bar") 1
0002 DO_ICALL
0003 RETURN int(1)
string(25) "php-src/test.php: foo-bar"

Choose a reason for hiding this comment

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

Fair enough. This can always be added later. And if opcache takes care of it, it's likely fairly unimportant.

@TimWolla TimWolla requested a review from iluuu1994 June 6, 2024 12:19
@TimWolla TimWolla force-pushed the sprintf-optimize branch 2 times, most recently from 372b967 to b6f1fb1 Compare June 12, 2024 08:07
TimWolla and others added 3 commits June 12, 2024 10:08
This just extracts the implementation as-is into a dedicated function to make
it reusable in preparation of a future commit.
… a single constant string

Without this Opcache will trigger a use-after-free in
`zend_optimizer_compact_literals()`.

Co-authored-by: Ilija Tovilo <[email protected]>
@TimWolla TimWolla force-pushed the sprintf-optimize branch 5 times, most recently from f052bde to 37127c2 Compare June 12, 2024 09:35
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.
@TimWolla TimWolla closed this Jun 12, 2024
TimWolla pushed a commit that referenced this pull request Oct 13, 2024
even without sanitizers, it is reproducible but with the following

```
<?php
$g = gmp_init(256);
var_dump(gmp_pow($g, PHP_INT_MAX));
```

we get this

```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==286922==ERROR: AddressSanitizer: FPE on unknown address 0x03e8000460ca (pc 0x7faf6c69de5c bp 0x400000000000004 sp 0x7ffe9843c740 T0)
    #0 0x7faf6c69de5c in __pthread_kill_implementation nptl/pthread_kill.c:44
    #1 0x7faf6c649c81 in __GI_raise ../sysdeps/posix/raise.c:26
    #2 0x7faf6db9386c in __gmp_exception (/lib/x86_64-linux-gnu/libgmp.so.10+0xd86c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #3 0x7faf6db938d3 in __gmp_overflow_in_mpz (/lib/x86_64-linux-gnu/libgmp.so.10+0xd8d3) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #4 0x7faf6dbac95c in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x2695c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #5 0x7faf6dba9038 in __gmpz_n_pow_ui (/lib/x86_64-linux-gnu/libgmp.so.10+0x23038) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #6 0x5565ae1ccd9f in zif_gmp_pow /home/dcarlier/Contribs/php-src/ext/gmp/gmp.c:1286
    php#7 0x5565aee96ea9 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1312
    php#8 0x5565af144320 in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:56075
    php#9 0x5565af160f07 in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:60439
    php#10 0x5565aed6fafe in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1842
    php#11 0x5565aeae70a8 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2578
    php#12 0x5565af532f4e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:964
    php#13 0x5565af535877 in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1334
    php#14 0x7faf6c633d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    php#15 0x7faf6c633e24 in __libc_start_main_impl ../csu/libc-start.c:360
    php#16 0x5565adc04040 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2604040) (BuildId: 949049955bdf8b7197390b1978a1dfc3ef6fdf38)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE nptl/pthread_kill.c:44 in __pthread_kill_implementation
==286922==ABORTING
```
TimWolla pushed a commit that referenced this pull request Oct 28, 2024
even without sanitizers, it is reproducible but with the following

```
<?php
$g = gmp_init(256);
var_dump(gmp_pow($g, PHP_INT_MAX));
```

we get this

```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==286922==ERROR: AddressSanitizer: FPE on unknown address 0x03e8000460ca (pc 0x7faf6c69de5c bp 0x400000000000004 sp 0x7ffe9843c740 T0)
    #0 0x7faf6c69de5c in __pthread_kill_implementation nptl/pthread_kill.c:44
    #1 0x7faf6c649c81 in __GI_raise ../sysdeps/posix/raise.c:26
    #2 0x7faf6db9386c in __gmp_exception (/lib/x86_64-linux-gnu/libgmp.so.10+0xd86c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #3 0x7faf6db938d3 in __gmp_overflow_in_mpz (/lib/x86_64-linux-gnu/libgmp.so.10+0xd8d3) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #4 0x7faf6dbac95c in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x2695c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #5 0x7faf6dba9038 in __gmpz_n_pow_ui (/lib/x86_64-linux-gnu/libgmp.so.10+0x23038) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #6 0x5565ae1ccd9f in zif_gmp_pow /home/dcarlier/Contribs/php-src/ext/gmp/gmp.c:1286
    php#7 0x5565aee96ea9 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1312
    php#8 0x5565af144320 in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:56075
    php#9 0x5565af160f07 in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:60439
    php#10 0x5565aed6fafe in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1842
    php#11 0x5565aeae70a8 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2578
    php#12 0x5565af532f4e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:964
    php#13 0x5565af535877 in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1334
    php#14 0x7faf6c633d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    php#15 0x7faf6c633e24 in __libc_start_main_impl ../csu/libc-start.c:360
    php#16 0x5565adc04040 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2604040) (BuildId: 949049955bdf8b7197390b1978a1dfc3ef6fdf38)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE nptl/pthread_kill.c:44 in __pthread_kill_implementation
==286922==ABORTING
```

close phpGH-16384
TimWolla pushed a commit that referenced this pull request Mar 29, 2025
```
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
    #0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
    #1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
    #2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
    #3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
    #4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
    #5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
    #6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
    php#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
    php#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
    php#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
    php#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    php#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
    php#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)
```

close phpGH-18006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants