Skip to content

optimize pack #18513

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

optimize pack #18513

wants to merge 1 commit into from

Conversation

divinity76
Copy link
Contributor

@divinity76 divinity76 commented May 7, 2025

got rid of BASIC_MINIT_SUBMODULE(pack)
by moving a bunch of stuff from runtime-initialization to compile-time initialization.

hans@DESKTOP-EE15SLU:~/projects/php-src$ cat bench.php
<?php

for ($i = 0; $i < 10_000_000; ++$i) {
    pack("J", 0x7FFFFFFFFFFFFFFF);
}
hans@DESKTOP-EE15SLU:~/projects/php-src$ hyperfine --warmup 10 --runs 100 './sapi/cli/php-unoptimized ./bench.php' ; hyperfine --warmup 10 --runs 100 './sapi/cli/php-optimized ./bench.php' ;
Benchmark 1: ./sapi/cli/php-unoptimized ./bench.php
  Time (mean ± σ):     261.7 ms ±  32.6 ms    [User: 252.3 ms, System: 1.3 ms]
  Range (min … max):   249.6 ms … 414.5 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 1: ./sapi/cli/php-optimized ./bench.php
  Time (mean ± σ):     259.4 ms ±  25.5 ms    [User: 250.4 ms, System: 0.9 ms]
  Range (min … max):   248.8 ms … 401.9 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
  • don't have access to a quiet system
  • don't have access to a big-endian system to test on. hopefully Github CI does.

got rid of BASIC_MINIT_SUBMODULE(pack)
by moving a bunch of stuff from runtime-initialization
to compile-time initialization.

before/after:
hans@DESKTOP-EE15SLU:~/projects/php-src$ cat bench.php
<?php

for ($i = 0; $i < 10_000_000; ++$i) {
    pack("J", 0x7FFFFFFFFFFFFFFF);
}

before/after:
hans@DESKTOP-EE15SLU:~/projects/php-src$ hyperfine --warmup 10 --runs 100 './sapi/cli/php-unoptimized ./bench.php' ; hyperfine --warmup 10 --runs 100 './sapi/cli/php-optimized ./bench.php' ;
Benchmark 1: ./sapi/cli/php-unoptimized ./bench.php
  Time (mean ± σ):     261.7 ms ±  32.6 ms    [User: 252.3 ms, System: 1.3 ms]
  Range (min … max):   249.6 ms … 414.5 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 1: ./sapi/cli/php-optimized ./bench.php
  Time (mean ± σ):     259.4 ms ±  25.5 ms    [User: 250.4 ms, System: 0.9 ms]
  Range (min … max):   248.8 ms … 401.9 ms    100 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

-i don't have access to a quiet system :'(
-i don't have access to a big-endian system to test on. hopefully Github CI tests does.
@nielsdos
Copy link
Member

nielsdos commented May 7, 2025

MINIT is only executed once at startup of PHP, which is why the difference is so small. So this likely only has a real benefit on CLI, if at all. Not against this though as this looks a bit simpler, but I'll let bukka judge.

don't have access to a big-endian system to test on. hopefully Github CI does.

Nightly has a BE job.


I was also looking into optimizing pack() after I saw your docs PR. I noted that getting rid of convert_to_long provided a nice benefit and I have a local patch for that. I'll submit that later today.

@nielsdos
Copy link
Member

nielsdos commented May 7, 2025

I believe it's possible to get rid of the mapping arrays altogether and just use some byte swapping, didn't test on a big endian system yet however: https://gist.github.com/nielsdos/db426e7776a7e73fc0b953c082d91e76
This should also be faster than relying on a map

@divinity76
Copy link
Contributor Author

I believe it's possible to get rid of the mapping arrays altogether and just use some byte swapping, didn't test on a big endian system yet however: https://gist.github.com/nielsdos/db426e7776a7e73fc0b953c082d91e76 This should also be faster than relying on a map

ooo that's much better

@divinity76
Copy link
Contributor Author

btw I wonder at what point big-endian becomes so obscure that support can be dropped entirely,

does anyone actually run PHP on a big-endian system?

@nielsdos
Copy link
Member

nielsdos commented May 7, 2025

btw I wonder at what point big-endian becomes so obscure that support can be dropped entirely,

Some protocols may use big endian in which case you'll still want to have this support.

does anyone actually run PHP on a big-endian system?

Apparently yes, not too long ago we fixed some issues with that+FFI.

@divinity76
Copy link
Contributor Author

Some protocols may use big endian in which case you'll still want to have this support.

I didn't mean "remove little-endian-php's ability to parse big-endian bytes", I meant more like "remove php-src's compile-on-big-endian compatibility stuff"

Apparently yes, not too long ago we fixed some issues with that+FFI.

Interesting, do you remember the title/issue/link?

@Girgias
Copy link
Member

Girgias commented May 8, 2025

btw I wonder at what point big-endian becomes so obscure that support can be dropped entirely,

does anyone actually run PHP on a big-endian system?

People running PHP on IBM Z mainframes, we used to have an S390X CI pipeline on Travis before that company made it impossible for us to use them. And some IBM folks do still submit fixes when they encounter bugs.

@nielsdos
Copy link
Member

nielsdos commented May 8, 2025

Some protocols may use big endian in which case you'll still want to have this support.

I didn't mean "remove little-endian-php's ability to parse big-endian bytes", I meant more like "remove php-src's compile-on-big-endian compatibility stuff"

Ah right. Often, there isn't really a lot of extra code necessary in PHP to support big endian machines, so it's not too big of a burden. We have some here, in bcmath and in phar, but they're not a lot.

Apparently yes, not too long ago we fixed some issues with that+FFI.

Interesting, do you remember the title/issue/link?

This was one of them: #16013

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