Repository navigation
Conversation
|
Cross-posting from mailing list to get a reminder on discussion here: Hello Matthieu, If the primary concern is with size, it may make sense to instead look into creating a reusable shared library on unix, much like on Windows. That being said, while I'm also in favour of shipping only one binary for both, I strongly believe that it should not become an opt-in that nobody ends up shipping. It should become default at the very least, with an option to explicitly disable cli/fpm sources to save a few kb in a 9mb+++ binary at most. Have you had any thoughts about the embed shared library if fpm should also be exposed there? Marc |
bukka
left a comment
There was a problem hiding this comment.
I'm not sure about that splitting - we should for sure check with packagers to see what they think.
| {'D', 0, "daemonize"}, | ||
| {'F', 0, "nodaemonize"}, | ||
| {'O', 0, "force-stderr"}, | ||
| {20, 0, "fpm"}, /* "php --fpm", see sapi/cli/php_cli_main.c */ |
There was a problem hiding this comment.
I don't understand this - those are options for php-fpm binary so why is it added here?
There was a problem hiding this comment.
It's there because php --fpm hands its argv to FPM unchanged, so FPM's getopt sees --fpm. The argv has to stay intact: on reload fpm_pctl_exec() re-executes the saved argv, so it must still start with php --fpm, and the process title is written over the original argv area.
But you're right that FPM's option table shouldn't know about this. The caller now tells FPM where its options start, so the entry is gone and php-fpm --fpm is rejected again.
|
|
||
| int fpm_run(int *max_requests); | ||
| /* this performs full fpm-SAPI boot: the former main() of php-fpm. */ | ||
| int do_php_fpm(int argc, char *argv[]); |
There was a problem hiding this comment.
fpm function should be prefix with fpm_
There was a problem hiding this comment.
I don't agree here, unless we also rename do_php_cli to cli_...
There was a problem hiding this comment.
Yeah cli should rename as well - it doesn't match our convention. Especially in fpm the convention is quite strict to prefix all functions with fpm_ .
There was a problem hiding this comment.
Renamed to fpm_main(). For do_php_cli(): since it's exported for embed and ships in 8.6, I'm leaving it out of this PR (LMK if you want me to take any action).
| dnl Everything except the main() entry point, so that the CLI binary can link | ||
| dnl the same objects for do_php_fpm() (--enable-cli-fpm). | ||
| PHP_ADD_SOURCES_X([sapi/fpm], | ||
| [$PHP_FPM_FILES $PHP_FPM_TRACE_FILES $PHP_FPM_SD_FILES], | ||
| [-I$abs_srcdir/sapi/fpm -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1], | ||
| [PHP_FPM_SHARED_OBJS]) | ||
| PHP_FPM_OBJS="$PHP_FPM_OBJS $PHP_FPM_SHARED_OBJS" | ||
| PHP_SUBST([PHP_FPM_SHARED_OBJS]) |
There was a problem hiding this comment.
Are you I'm not sure this is a good idea. Does this mean that it will always created shared lib for php-fpm so it will no longer be a single binary?
There was a problem hiding this comment.
No shared library is built. "SHARED" means shared between two SAPIs, following PHP_CLI_SHARED_OBJS naming from #21385. Happy to rename it (e.g. PHP_FPM_COMMON_OBJS) if you find it confusing.
As a build maintainer, especially with largely static linkage, I think it's a great idea. The different functionality could also be exposed directly if the binary is invoked under the |
47a27e7 to
5c50562
Compare
|
Thanks for the review!
I don't see a direct use case for putting FPM in the embed library (from my POV) so I won't push for it in this change. But it could be a follow-up PR.
If "splitting" refers to the config.m4 change, see my reply above: no shared library is built, php-fpm is still a single binary. If it's about the idea of putting FPM in the embed library or something else let me know.
Good idea, done: in a combined build, running the binary under a name starting with
I'd love to make this the default. I might include that as a vote in the RFC? (let me know if that's a decision that can instead be made here in this PR) |
|
RFC published: https://wiki.php.net/rfc/single-binary-cli-fpm I also started a discussion thread in the mailing list. |
This adds an opt-in configure option,
--enable-cli-fpm, that links the FPM SAPI into thephpbinary.RFC
Intro
The binary behaves exactly like the CLI unless its first argument is
--fpm, or it is invoked under a name starting withphp-fpm(e.g. aphp-fpmsymlink tophp). In both cases it runs the php-fpm master with the remaining arguments:The default build is unchanged: without the flag,
phpandphp-fpmare built exactly as before.Goal
In some environments, the size of the runtime matters. Having
phpandphp-fpmbinaries when they are ~99% the same code can be wasteful.That's the case for example on AWS Lambda with Bref, where a second ~24 MB binary on disk increases the cold start duration (because that's more data to load in the container/micro-VM when it starts).
Design
This follows the shape of #21385 (
do_php_cli()/PHP_CLI_SHARED_OBJSfor embed):sapi/fpm/fpm/fpm_main.c:main()becomesfpm_main(argc, argv, first_arg), declared infpm.h. A newsapi/fpm/php_fpm_main.cholds the one-linemain()for the standalonephp-fpmbinary.sapi/fpm/config.m4:PHP_SELECT_SAPInow only takesphp_fpm_main.c; all other FPM sources go throughPHP_ADD_SOURCES_XintoPHP_FPM_SHARED_OBJS, which is appended toPHP_FPM_OBJS. These are plain objects, no shared library is built, and theBUILD_FPMlink lines are untouched.sapi/fpm/config0.m4(new):PHP_ARG_ENABLE([fpm])moves here so$PHP_FPMis set beforesapi/cli/config.m4runs (same reason embed has aconfig0.m4).sapi/cli/config.m4:PHP_ARG_ENABLE([cli-fpm]), default off; errors out unless both the CLI and FPM SAPIs are enabled; definesPHP_CLI_WITH_FPMand appends$(PHP_FASTCGI_OBJS) $(PHP_FPM_SHARED_OBJS)toPHP_CLI_OBJS. TheBUILD_CLIlines are untouched.FPM_EXTRA_LIBS(systemd, acl, apparmor, selinux) is appended toEXTRA_LIBSwhen the flag is on.sapi/cli/php_cli_main.c: under#ifdef PHP_CLI_WITH_FPM, callsfpm_main()whenbasename(argv[0])starts withphp-fpm(FPM options start atargv[1]), or whenargv[1]is--fpm(FPM options start atargv[2]).php --fpmor thephp-fpmname), and the process title is written over the original argv area. FPM's option table is untouched, sophp-fpm --fpmis still rejected.PHP_FUNCTION(apache_request_headers), which is a duplicate-symbol link error when linked together. FPM's C symbol is renamed tofpm_request_headers; the stub uses@implementation-aliason bothapache_request_headers()andgetallheaders()and the arginfo header is regenerated. Userland names and behaviour are unchanged.php --help,php.1, NEWS, UPGRADING and UPGRADING.INTERNALS are updated.sapi/cli/tests/cli_fpm.phptskips unless the flag is built in. The wholesapi/fpm/testssuite also passes when run throughphp --fpmand through aphp-fpmsymlink tophp(including the reload tests).Backward compatibility
main()of php-fpm is nowfpm_main();--enable-fpmis declared inconfig0.m4. Both noted in UPGRADING.INTERNALS.