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

[DI] dump factory files as classes #36193

Merged
merged 1 commit into from Mar 31, 2020

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 24, 2020

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR is a performance improvement when using bin/console on the command line.

Once upon a time, we advised setting container.dumper.inline_factories to false so that the container could be chunked into many files. More recently, we turned this setting back to true in order to optimize for preloading. But this made bin/console back to slow: since the CLI cannot have opcache, PHP has to parse this potentially big file all the time. Previous data already showed this can grow big.

This PR fixes the issue by generating many files again. But instead of generating the inline code within each file, we now wrap this code inside a class. Then we list this class for preloading.

This way, we have the best of both worlds: a bin/console that scales no matter the size of the app and top perf when using preloading (I benched a small hello world before/after the patch with preloading enabled, there is no measurable difference.)

This should also fix a memory leak that happens when factory files contain closures.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 24, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.0 Mar 24, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-dump-classes branch from 97d12a9 to eb54c88 Mar 24, 2020
@nicolas-grekas nicolas-grekas modified the milestones: 5.0, next Mar 24, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-dump-classes branch 2 times, most recently from c14f919 to 5f6bff5 Mar 24, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-dump-classes branch from 5f6bff5 to cedb5cd Mar 25, 2020
@nicolas-grekas nicolas-grekas merged commit 0876480 into symfony:master Mar 31, 2020
2 of 3 checks passed
2 of 3 checks passed
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-dump-classes branch Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.