Do not compile in parallel #723
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theofidry
added a commit
that referenced
this pull request
Nov 27, 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.**
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think there is some serious issues, either related to amphp or laravel/closure. In any case currently I see a x1000 speed difference...