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] Allow dumping the container in one file instead of many files #32581

Merged
merged 1 commit into from Jul 31, 2019

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 17, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Replaces #32119
As spotted by @lyrixx, putting all service factories in one big container can be easier to manage with workers. It could also play well with PHP7.4's preloading.

This PR adds a container.dumper.inline_factories parameter to enable this behavior.
When it is set to true, a single big container file is created.

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 17, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:dic-no-files branch from 143a701 to 256e765 Jul 17, 2019
@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jul 17, 2019

This will need some tests. At least functional tests, to unsure we can boot() the kernel (several times)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:dic-no-files branch 3 times, most recently from 4c1a9b2 to 685ff7f Jul 17, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:dic-no-files branch from 685ff7f to c893986 Jul 17, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jul 17, 2019

Unit tests added.

At least functional tests, to unsure we can boot() the kernel (several times)

not added because this would be unrelated: this PR changes absolutely nothing in terms of how the container is loaded.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jul 19, 2019

What are the pros/cons of setting container.dumper.inline_factories to true/false? Do we really need two modes?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jul 19, 2019

We demonstrated in the past that loading a set of smaller files is faster than loading a single big container file. The drawback that @lyrixx experienced is found on workers: because they are long-running, they are more likely to be affected by the removal of one of those files. This doesn't happen for requests that reload everything all the time.

What is unknown is how PHP 7.4 will change the situation regarding performance. With preloading, a single big file can, in theory, be similarly fast or faster. But anyway, we're not going to support only PHP 7.4. If that's confirmed, we'll be able to have a recipe that sets this param to true on PHP 7.4, and drop that in Symfony 6.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Jul 19, 2019

The drawback that @lyrixx experienced is found on workers: because they are long-running, they are more likely to be affected by the removal of one of those files.

Why would a file be removed? Sounds like a deployment problem.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jul 19, 2019

On dev I suppose, it disrupts the workflow.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jul 19, 2019

Yes it's in development of course 👍
As soon as you change a constructor the cache is built again
So running consumers explode

@lyrixx
lyrixx approved these changes Jul 23, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jul 31, 2019

I'm merging this to move forward. We might consider defaulting or sticking to one way or another, but we need to be able to compare first.

@nicolas-grekas nicolas-grekas merged commit c893986 into symfony:4.4 Jul 31, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
nicolas-grekas added a commit that referenced this pull request Jul 31, 2019
…f many files (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DI] Allow dumping the container in one file instead of many files

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Replaces #32119
As spotted by @lyrixx, putting all service factories in one big container can be easier to manage with workers. It could also play well with PHP7.4's preloading.

This PR adds a `container.dumper.inline_factories` parameter to enable this behavior.
When it is set to true, a single big container file is created.

Commits
-------

c893986 [DI] Allow dumping the container in one file instead of many files
@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Aug 1, 2019

Thanks @nicolas-grekas for this one :)

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:dic-no-files branch Aug 1, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.