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

fix: Migrate from amphp/parallel-functions to amphp/parallel #1216

Merged
merged 10 commits into from
Nov 27, 2023

Conversation

theofidry
Copy link
Member

@theofidry theofidry commented Nov 26, 2023

In #723 I disabled the parallelization by default as building the Box PHAR with parallelization was x1000 slower than without.

Since then, a number of things changed in PHP-Scoper and in preparation to adapt the way the parallelization is done, a few things changed in Box too. I am not sure what element made a difference but according to PHPBench, enabling parallelization was reducing the build time from 13.932s to 9.243s so a ~33% speed improvement.

I however gave it more though I as thinking serializing closures is a bad idea of a source of too many problems. Currently using parallelization in Box results in errors due to data being readonly. I do not remember exactly in what part laravel/serializable-closure messes up but I also remembered it was a limitation of the library (and in fairness, the library is really not at fault, it tries really hard to hack its way through to patch a very annoying PHP limitation, so it does what it can).

But I kept finding those issues too annoying and I think the solution is also quite simple: to use AMPHP at a level lower which would avoid to have to serialize closures altogether.

Closes #552.
Closes #602 (the memory issue seems gone according to the PHPBench results).
Closes #1160 (superseded).

With those changes, we get the following results:

subject memory mode rstdev stdev
with PHP-Scoper; no parallel processing 940.391mb 13.932s ±0.88% 122.973ms
with PHP-Scoper; parallel processing 47.957mb 6.668s ±0.64% 42.899ms

In other words the parallelization goes from 9.243s to 6.668s, so a further ~28% improvement making it ~52% faster than without parallelization.

@theofidry theofidry marked this pull request as ready for review November 26, 2023 21:43
@theofidry theofidry enabled auto-merge (squash) November 26, 2023 21:45
@theofidry theofidry changed the title fix: Make parallelization work fix: Migrate from amphp/parallel-functions to amphp/parallel Nov 27, 2023
@theofidry theofidry merged commit 0384de9 into box-project:main Nov 27, 2023
155 of 157 checks passed
@theofidry theofidry deleted the refactor/amp-worker branch November 27, 2023 07:06
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.

PhpScoper compactor seems to run into memory leak Optimize no-parallel run
1 participant