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

[Console] Symfony Console memory usage increased after upgrade to Symfony 3.3 #23601

Closed
umpirsky opened this issue Jul 20, 2017 · 18 comments
Closed

Comments

@umpirsky
Copy link
Contributor

Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.3.5

There is a noticable memory increase in console Application initialization after the upgrade from v3.1.10 to v3.3.5.

screenshot from 2017-07-20 15-24-24
screenshot from 2017-07-20 15-24-39

Were there any changes in recent version that may be causing this? I saw #22900, but does not look related, I tried removing add() method and got similar results.

@nicolas-grekas
Copy link
Member

I guess the full Blackfire comparison profile would help, can you provide it please? (even in private eg on Slack)

@umpirsky
Copy link
Contributor Author

@nicolas-grekas Sure, I can share it privately, can you provide link to slack org or email address please? Thanks!

@nicolas-grekas
Copy link
Member

@nicolas-grekas
Copy link
Member

I looked at your profile, and the issue comes from
Symfony\Component\HttpKernel\Kernel::initializeContainer, with a cost increase related to 3 items:

  • Symfony\Component\Config\ConfigCache::isFresh (+64ms)
  • compile::qa/appQaDebugProjectContainer.php (+40ms)
  • run::qa/appQaDebugProjectContainer.php (+8ms)

The total (+112ms) holds almost 100% of the wall time delta.
This means that the container is bigger (thus it takes longer to check for freshness + parse + execute).

I can't say if it's legitimate or not without the app. You should compare the dumped container in 3.1 and 3.3 and see if you can suggest any fix/issue.

@umpirsky
Copy link
Contributor Author

umpirsky commented Jul 24, 2017

@nicolas-grekas Thanks for taking time to check this. 👍

I checked compiled containers and there is a slight increase in dumped container size (3.1M -> 3.7M). The reason for this is simple increase (3%) of number of services with the new Symfony version.

However, I'm not concerned about wall time increase as much as I am about memory increase in:

screenshot from 2017-07-20 15-24-39

That's the problem I am interested in and it does seem to come from the same direction - initializeContainer!
But I'm not sure I can justify such a huge memory consumption increase.

This:

screenshot from 2017-07-24 17-31-08

also shows that memory consumption is moved from run to registering commands, which is fine, but still, commands need to be registered (unless lazy iirc) before running the app. Also, it sums up to 6.85M more in total.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 24, 2017

memory consumption is moved from run to registering commands

this is just the result of architectural change, don't focus on that, as the graph shows that real memory increase comes from the common node: the boot method. That's where you need to focus (click on the magnifying glass to redraw the graph around that node, etc.)

But is 7Mb really an issue?

@umpirsky
Copy link
Contributor Author

But is 7Mb really an issue?

@nicolas-grekas It is. The way we noticed this is CPU spike on the server after the upgrade is deployed. Our guess is that it comes from resque jobs, it forks to process jobs. Memory size is the biggest factor when it comes to forking taking CPU. It takes CPU to make that copy, and the amount of CPU it takes is based on the total size of the memory.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 28, 2017

Dunno what we can do here, for now, I'd say nothing... If the container is bigger, it takes more memory...
BTW, the isFresh check means you're running in env=dev (or debug=true), isn't that the real issue?

@umpirsky
Copy link
Contributor Author

@nicolas-grekas I will investigate this further. yes, this profiles are done in debug mode, but actual issue was present with debug=false. I'll post more findings if I have them, thanks for your help so far, really appreciate it!

@umpirsky
Copy link
Contributor Author

I profiled again ./app/console --no-debug with v3.1.10 and v3.3.5.

Memory consumed by AppKernel::initializeContainer() increased from 15M to 22M while number of services went from 1761 to 1803.
That's 32% memory increase for 3% number of service increase.
appQaProjectContainer.php size grew from 2933889 to 3560866.

Results are similar with 3.3@dev.

screenshot from 2017-07-28 22-36-23

@nicolas-grekas
Copy link
Member

@umpirsky can you please test with #23741?

fabpot added a commit that referenced this issue Aug 7, 2017
…ekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Generate one file per service factory

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

See #23678 for background on this proposal.

Commits
-------

4037009 [DI] Generate one file per service factory
@umpirsky
Copy link
Contributor Author

@nicolas-grekas Will do in coming days, thanks!

@umpirsky
Copy link
Contributor Author

Blocked by #23924.

@chalasr
Copy link
Member

chalasr commented Aug 22, 2017

#23924 has been fixed.

@umpirsky
Copy link
Contributor Author

Good news everyone!

I profiled with latest 3.4.x-dev 2b4b224 version.

The results are awesome on the first look, great work @nicolas-grekas!

Here is how AppKernel::initializeContainer() compares now from v3.1.10 to 3.4.x-dev 2b4b224:

screenshot from 2017-08-30 13-00-32

All we need to do now is wait for 3.4 stable. :)

@umpirsky
Copy link
Contributor Author

@nicolas-grekas Is there a chance to get this optimizations in Symfony 3.3?

@stof
Copy link
Member

stof commented Aug 30, 2017

@umpirsky we won't backport such big refactoring into a patch release. This is too risky (and it actually relies on new features)

@nicolas-grekas
Copy link
Member

Amazing, thanks for providing such a clear proof.
Closing then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants