Skip to content
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

Performance degredation #385

Closed
mpociot opened this issue Mar 15, 2024 · 10 comments · Fixed by #403
Closed

Performance degredation #385

mpociot opened this issue Mar 15, 2024 · 10 comments · Fixed by #403
Labels
bug Something isn't working kind/php-and-sapi Issues related to php source and SAPI
Milestone

Comments

@mpociot
Copy link
Contributor

mpociot commented Mar 15, 2024

For Laravel Herd, we compile the current versions of PHP with a version of static-php-cli from December 2023.

After upgrading to the latest version on the main branch, I notice a significant performance decrease with the compiled version of PHP.

I noticed this by running the Laravel 11 test suite. One test in particular https://github.com/laravel/framework/blob/11.x/tests/Support/SupportStrTest.php#L589-L602

Used extensions/build parameters:

intl,pdo_sqlite,sqlite3,curl,openssl,tokenizer,bcmath,bz2,calendar,dba,ftp,iconv,mysqli,mbstring,mbregex,xml,simplexml,ctype,dom,pdo,filter,session,zlib,fileinfo,pdo_mysql,posix,sockets,shmop,sodium,sysvmsg,sysvsem,sysvshm,gd,zip,gmp,redis,xmlwriter,phar,exif,xmlreader,readline,pcntl,soap,imagick,ffi,password-argon2,pgsql,pdo_pgsql,imap,ldap,xsl --debug --build-cli  --build-fpm --with-libs=nghttp2 --no-strip

To reproduce the issue:

git clone https://github.com/laravel/framework.git
cd framework
composer update
php vendor/bin/phpunit tests/Support/SupportStrTest.php --filter=testWhetherTheNumberOfGeneratedCharactersIsEquallyDistributed

Test results

PHP 8.3.4 compiled with the version of static-php-cli from December 2023:

Time: 00:00.264, Memory: 10.00 MB

PHP 8.3.4 compiled with the current main branch

Time: 00:00.777, Memory: 10.00 MB

Edit: I'll go through some tagged versions locally using the build arguments to see which version introduced this bug

@crazywhalecc crazywhalecc added bug Something isn't working kind/php-and-sapi Issues related to php source and SAPI labels Mar 15, 2024
@crazywhalecc
Copy link
Owner

crazywhalecc commented Mar 15, 2024

After 2.1.0 (released 3 weeks ago), we have static-php-cli.version PHP info injection. Just use php -i | grep static-php-cli to see which version. But based on your description, Herd is most likely using a version before 2.0.0 release.

@mpociot
Copy link
Contributor Author

mpociot commented Mar 15, 2024

So far I tested two points in time where the performance issue is already present. As I can't build tag 2.0.1 because of an issue with libxml that got patched between 2.0.1 and 2.1.0, I had to use a specific commit hash.

These versions already have the performance degredation in place:

❌ Tag 2.1.0
❌ Commit #304

✅ The last upstream commit that we use on Herd was this: cf198e0

Building PHP 8.3.4 on this specific commit does not have the performance issue.

So the issue seems to have been introduced between the 2.0.0 RC and the stable 2.0.0 release.

@crazywhalecc
Copy link
Owner

crazywhalecc commented Mar 16, 2024

Oh I found it. Just because Herd is using --no-strip, and when no strip, the optimization flag will be -g -O0 (default is -g0 -Os). In the main branch, I disabled optimizations for debugging purposes.

Maybe it's time to resolve #377 and #382 (for passing in custom compilation flags) as soon as possible. 🤔

@mpociot
Copy link
Contributor Author

mpociot commented Mar 16, 2024

Ah yeah. Well I also wouldn't mind if there would be a simple argument to re-add the optimization flags 🤷‍♂️
Thank you for looking into this

@crazywhalecc crazywhalecc added this to the 2.2 Release milestone Mar 20, 2024
@crazywhalecc
Copy link
Owner

After a week of various attempts, I finally decided with difficulty to give up this plan: change all parameters to environment variables.

Mainly because I can't figure out which ones need to be customized frequently and which ones don't. There is no scope here. For this topic only, I think we can maintain a customizable compilation parameter variable table in the document, for example:

var name default value comment
SPC_PHP_EXTRA_CFLAGS -g -Os used in php-src's make EXTRA_CFLAGS={$vars}

@mpociot
Copy link
Contributor Author

mpociot commented Apr 6, 2024

Sounds good to me @crazywhalecc
Does that mean that once this is implemented, I can simply rely on the default values?

@crazywhalecc
Copy link
Owner

crazywhalecc commented Apr 6, 2024

@mpociot I won't change current build flags. For Laravel Herd macOS, the best way is just using export SPC_CMD_VAR_PHP_MAKE_EXTRA_CFLAGS="-g -Os" and --no-strip. (for future, add to CI's env?)

This update will be completed within a week and will have 2.2.0 as the version number.

SPC_CMD_VAR_PHP_MAKE_EXTRA_CFLAGS this name is a bit long because many environment variables may need to be introduced at present. I will also write these into the static-php.dev document.

@mpociot
Copy link
Contributor Author

mpociot commented Apr 6, 2024

Great, thank you! Is this already implemented?

@crazywhalecc
Copy link
Owner

WIP, but env vars for MacOSBuilder have already finished. I'm finishing up the Linux part now.

@crazywhalecc
Copy link
Owner

Finished, docs: https://static-php.dev/en/guide/env-vars.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kind/php-and-sapi Issues related to php source and SAPI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants