Skip to content

Conversation

@PauliusSap
Copy link
Contributor

@PauliusSap PauliusSap commented Mar 5, 2021

Problem is when you need spawn very fast a lot of fpm child processes in the dynamic mode (pm = dynamic).
After every spawn it's make 1000ms pause (i think it's HEARTBEAT checker).
Child processes spawn algorithm growth x2 while exceed maximum spawn rate at this moment hardcoded value 32.
When you need spawn a lot of child processes you take a lot of time.

Example we have already spawned 3000 child processes at high load traffic need spawn fast +900 child processes.

[00:00:01] spawning 8 children, there are 0 idle, and 3008 total children 
[00:00:02] spawning 16 children, there are 0 idle, and 3024 total children <!-- algorithm growth x2
[00:00:03] spawning 32 children, there are 0 idle, and 3056 total children <!-- algorithm growth x2
[00:00:04] spawning 32 children, there are 0 idle, and 3088 total children <!-- reached maximum from this moment spawn only 32.
[00:00:05] spawning 32 children, there are 0 idle, and 3120 total children
...
...
[00:00:30] spawning 32 children, there are 0 idle, and 3920 total children <!-- Total exceed 30 seconds

Result is: we wait 30 seconds to spawn +920 child processes.

Summary: it's good to have this setting in the configuration because spawn new child processes don't have some heavy overhead.

@staabm
Copy link
Contributor

staabm commented Mar 5, 2021

Could you elaborate why/when you need to adjust this setting?

@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch from eac4b63 to 8ac8e6e Compare March 5, 2021 19:32
@nikic nikic requested a review from bukka March 5, 2021 20:05
@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch 7 times, most recently from 16abcc9 to ff2659b Compare March 6, 2021 13:51
@cmb69
Copy link
Member

cmb69 commented Mar 10, 2021

cc @bukka

@bukka
Copy link
Member

bukka commented Mar 10, 2021

It looks good and think it makes sense to have an option for this. The code looks good as well. Just one thing - could you please add some basic test? I realise that a properly test this might be a bit flaky or a bit too have for CI env so maybe something just basic to at least check that it's possible to set it.

@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch 18 times, most recently from 370e7b8 to 8f009b9 Compare March 14, 2021 10:13
@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch 4 times, most recently from 9b450ee to 5724f4e Compare March 14, 2021 16:21
@PauliusSap
Copy link
Contributor Author

It looks good and think it makes sense to have an option for this. The code looks good as well. Just one thing - could you please add some basic test? I realise that a properly test this might be a bit flaky or a bit too have for CI env so maybe something just basic to at least check that it's possible to set it.

@bukka I'm added a simple set test and merge commits.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Looks good. Just few minor things re the test.

@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch from 0f593e9 to 994ecb0 Compare March 22, 2021 20:16
@PauliusSap PauliusSap requested a review from bukka March 22, 2021 22:23
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Looks really good! Just few minor comments.

@bukka
Copy link
Member

bukka commented Mar 22, 2021

@PauliusSap Please could you then also squash it to a single commit.

@bukka
Copy link
Member

bukka commented Mar 22, 2021

And one more thing. Could you possibly also run fpm with this setting - possibly do one ping so it's kind of verified in the test that it's running. Something like extending config with

ping.path = /ping
ping.response = pong

and then add to the test something like

$tester = new FPM\Tester($cfg);
$tester->start();
$tester->expectLogStartNotices();
$tester->ping();
$tester->terminate();
$tester->expectLogTerminatingNotices();
$tester->close();

@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch 3 times, most recently from 025539b to 0999f33 Compare March 23, 2021 13:40
@PauliusSap
Copy link
Contributor Author

PauliusSap commented Mar 23, 2021

And one more thing. Could you possibly also run fpm with this setting - possibly do one ping so it's kind of verified in the test that it's running. Something like extending config with

ping.path = /ping
ping.response = pong

and then add to the test something like

$tester = new FPM\Tester($cfg);
$tester->start();
$tester->expectLogStartNotices();
$tester->ping();
$tester->terminate();
$tester->expectLogTerminatingNotices();
$tester->close();

I use test argument. start(['-t', '-t']);

-t, --test Test FPM configuration and exit

Second -t argument meaning dump config.

@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch 5 times, most recently from ba776f2 to dded5e2 Compare March 23, 2021 22:55
@PauliusSap PauliusSap requested a review from bukka March 23, 2021 23:31
* Add functionality to expect log config options
@PauliusSap PauliusSap force-pushed the feature/max-spawn-rate-config branch from dded5e2 to b9963f0 Compare March 25, 2021 10:49
@PauliusSap
Copy link
Contributor Author

@PauliusSap Please could you then also squash it to a single commit.

Done

@bukka
Copy link
Member

bukka commented Mar 28, 2021

Merged via eac1609

@bukka bukka closed this Mar 28, 2021
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Mar 9, 2022
> - FPM:
>   . Added new pool option for the dynamic process manager called
>     `pm.max_spawn_rate`. It allows to start number of children in a faster rate
>     when dynamic pm is selected. The default value is 32 which was the previous
>     hard coded value.

Includes unit tests.

Refs:
* https://github.com/php/php-src/blob/f67986a9218f4889d9352a87c29337a5b6eaa4bd/UPGRADING#L240-L243
* https://github.com/php/php-src/blob/33cd61c9045ae8675448456cdda6fb244b2efa01/NEWS#L280-L281
* php/php-src#6753
* php/php-src@eac1609
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.

4 participants