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] Add "container.hot_path" tag to flag the hot path and inline related services #24872

Merged
merged 1 commit into from Nov 9, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Nov 8, 2017

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

This PR is the result of my quest to squeeze some performance out of 3.4/4.0.

It builds on two ideas:

  • a new container.hot_path tag that identifies the services that are always needed. This tag is only applied to a very short list of bootstrapping services (router, event_dispatcher, http_kernel and request_stack only). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners.
  • replacing the PHP autoloader by plain inlined require_once in generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy.

The end result is significant, even on a simple Hello World.
Here is the Blackfire profile, results are consistent with ab benchmarks:

https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graph

capture du 2017-11-08 16-54-28

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Nov 8, 2017

Member

I like it.

But i think the name container.inline is misleading. It can be confused with the service inlining ($instance = new A(new B())).

IIUC, here it inlines the factory in the dumped container + it pre-load the PHP Class.

Member

lyrixx commented Nov 8, 2017

I like it.

But i think the name container.inline is misleading. It can be confused with the service inlining ($instance = new A(new B())).

IIUC, here it inlines the factory in the dumped container + it pre-load the PHP Class.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

container.hot_path?
ppl shouldn't really use that anyway, because propagation is enough most of the time

Member

nicolas-grekas commented Nov 8, 2017

container.hot_path?
ppl shouldn't really use that anyway, because propagation is enough most of the time

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Nov 8, 2017

Member

container.hot_path?

Yes it's better. And like that the name of the tag will be the same as the CompilerPass ;)

ppl shouldn't really use that anyway, because propagation is enough most of the time

It depends. We already use some inlining feature in Blackfire. It could be useful anyway.

Member

lyrixx commented Nov 8, 2017

container.hot_path?

Yes it's better. And like that the name of the tag will be the same as the CompilerPass ;)

ppl shouldn't really use that anyway, because propagation is enough most of the time

It depends. We already use some inlining feature in Blackfire. It could be useful anyway.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 8, 2017

Member

Tests are missing, for both the compiler pass and the new dumper behavior

Member

stof commented Nov 8, 2017

Tests are missing, for both the compiler pass and the new dumper behavior

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

Now with a new container.dumper.inline_class_loader parameter to opt-in this optimization, so that ppl doing strange things with their autoloader are still fine.

@stof thanks, comments addressed.

Member

nicolas-grekas commented Nov 8, 2017

Now with a new container.dumper.inline_class_loader parameter to opt-in this optimization, so that ppl doing strange things with their autoloader are still fine.

@stof thanks, comments addressed.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

Just proposed to enable the parameter by default in symfony/recipes#241

Member

nicolas-grekas commented Nov 8, 2017

Just proposed to enable the parameter by default in symfony/recipes#241

if (isset($lineage[$class])) {
return;
}
if (!$r = $this->container->getReflectionClass($class)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

@stof this still throws, but in a more controlled way. Since this is running late, all services should be fine xor the container is broken already. Do we want really to not throw? WDYT?

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

@stof this still throws, but in a more controlled way. Since this is running late, all services should be fine xor the container is broken already. Do we want really to not throw? WDYT?

@nicolas-grekas nicolas-grekas changed the title from [DI] Add "container.inline" tag to flag the hot path and inline related services to [DI] Add "container.hot_path" tag to flag the hot path and inline related services Nov 8, 2017

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Nov 8, 2017

Contributor

I feel container.inline is a little misleading, it feels something along the lines of container.require or container.preload is more correct naming wise. Otherwise i love the idea :)

Contributor

beberlei commented Nov 8, 2017

I feel container.inline is a little misleading, it feels something along the lines of container.require or container.preload is more correct naming wise. Otherwise i love the idea :)

@@ -114,10 +116,14 @@ public function dump(array $options = array())
'namespace' => '',
'as_files' => false,
'debug' => true,
'hot_path_tag' => null,

This comment has been minimized.

@Tobion

Tobion Nov 8, 2017

Member

Why does it not have a default?

@Tobion

Tobion Nov 8, 2017

Member

Why does it not have a default?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

I don't want it to be enabled by default, so this is opt-in.
This is the way to do it.

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

I don't want it to be enabled by default, so this is opt-in.
This is the way to do it.

@@ -1137,6 +1199,39 @@ private function addAliases()
return $code." );\n";
}
private function addInlineRequires()
{
if (!$this->hotPathTag || !$this->inlineRequires) {

This comment has been minimized.

@Tobion

Tobion Nov 8, 2017

Member

Why have two ways to disable this? Cant you just make hotPathTag required?

@Tobion

Tobion Nov 8, 2017

Member

Why have two ways to disable this? Cant you just make hotPathTag required?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

Because these are two independent settings (that play well together.)

@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

Because these are two independent settings (that play well together.)

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Nov 8, 2017

Member

So the router is loaded always even in CLI where it is not used?

Member

Tobion commented Nov 8, 2017

So the router is loaded always even in CLI where it is not used?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

The router classes are always loaded yes (not the objects). We don't care about the related CLI perf impact IMHO, it's negligible.

Member

nicolas-grekas commented Nov 8, 2017

The router classes are always loaded yes (not the objects). We don't care about the related CLI perf impact IMHO, it's negligible.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Nov 8, 2017

Member

Can't we preload the CLI classes as well then to speed up the CLI? If the impact of preloading unused classes is negligible, this shouldn't hurt webrequest as well.

Member

Tobion commented Nov 8, 2017

Can't we preload the CLI classes as well then to speed up the CLI? If the impact of preloading unused classes is negligible, this shouldn't hurt webrequest as well.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Member

There's nothing to optimize for the CLI: the CLI is not called 700 times per second as the web is. Just forking the CLI process is an order of magnitude slower than these optimizations.

Member

nicolas-grekas commented Nov 8, 2017

There's nothing to optimize for the CLI: the CLI is not called 700 times per second as the web is. Just forking the CLI process is an order of magnitude slower than these optimizations.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Nov 8, 2017

Contributor

On my biggest monolith app I can also measure a slight performance benefit for one html page that I tested 👍

  • prod, debug=false
  • composer dump --classmap-authoritative && app/console cache:clear --env=prod --no-debug
  • php 7.1, opcache enabled
  • ab -n 10000 -c 1 "http://..."

your branch HEAD~1 (8cd2193): 41.112 ms
your commit (44fe030) with enabled feature: 40.289 ms

So for me its roughly 2% faster. Not quite as significant as in your benchmarks. Under which conditions did you benchmark @nicolas-grekas ?

Contributor

dmaicher commented Nov 8, 2017

On my biggest monolith app I can also measure a slight performance benefit for one html page that I tested 👍

  • prod, debug=false
  • composer dump --classmap-authoritative && app/console cache:clear --env=prod --no-debug
  • php 7.1, opcache enabled
  • ab -n 10000 -c 1 "http://..."

your branch HEAD~1 (8cd2193): 41.112 ms
your commit (44fe030) with enabled feature: 40.289 ms

So for me its roughly 2% faster. Not quite as significant as in your benchmarks. Under which conditions did you benchmark @nicolas-grekas ?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 9, 2017

Member

Now with tests, PR ready for a last (hopefully) round of review.

@dmaicher thanks for the confirmation! My test was on an annotated controller with a Twig-rendered Hello World, and using Blackfire to profile. Using ab, I'm more at 2% also.

Member

nicolas-grekas commented Nov 9, 2017

Now with tests, PR ready for a last (hopefully) round of review.

@dmaicher thanks for the confirmation! My test was on an annotated controller with a Twig-rendered Hello World, and using Blackfire to profile. Using ab, I'm more at 2% also.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 9, 2017

Member

(failures are false-positives)

Member

nicolas-grekas commented Nov 9, 2017

(failures are false-positives)

$options = array_merge(array(
'class' => 'ProjectServiceContainer',
'base_class' => 'Container',
'namespace' => '',
'as_files' => false,
'debug' => true,
'hot_path_tag' => null,
'inline_class_loader_parameter' => 'container.dumper.inline_class_loader',

This comment has been minimized.

@lyrixx

lyrixx Nov 9, 2017

Member

Why is it configurable? Usually we don't want to add new options. IMHO, this is useless here.

@lyrixx

lyrixx Nov 9, 2017

Member

Why is it configurable? Usually we don't want to add new options. IMHO, this is useless here.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 9, 2017

Member

I don't like hardcoding a parameter name into the dumper

@nicolas-grekas

nicolas-grekas Nov 9, 2017

Member

I don't like hardcoding a parameter name into the dumper

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 9, 2017

Member

Can we make fabbot happy? Avoiding false-positives in the future would be great.

Member

fabpot commented Nov 9, 2017

Can we make fabbot happy? Avoiding false-positives in the future would be great.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 9, 2017

Member

green it is now :)

Member

nicolas-grekas commented Nov 9, 2017

green it is now :)

@fabpot

fabpot approved these changes Nov 9, 2017

@chalasr

chalasr approved these changes Nov 9, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 9, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Nov 9, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit f7cb559 into symfony:3.4 Nov 9, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Nov 9, 2017

minor #24872 [DI] Add "container.hot_path" tag to flag the hot path a…
…nd inline related services (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Add "container.hot_path" tag to flag the hot path and inline related services

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

This PR is the result of my quest to squeeze some performance out of 3.4/4.0.

It builds on two ideas:
- a new `container.inline` tag that identifies the services that are *always* needed. This tag is only applied to a very short list of bootstrapping services (`router`, `event_dispatcher`, `http_kernel` and `request_stack` only). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners.
- replacing the PHP autoloader by plain inlined `require_once` in generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy.

The end result is significant, even on a simple Hello World.
Here is the Blackfire profile, results are consistent with `ab` benchmarks:

https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graph

![capture du 2017-11-08 16-54-28](https://user-images.githubusercontent.com/243674/32558666-a3f439b2-c4a5-11e7-83a3-db588c3e21e5.png)

Commits
-------

f7cb559 [DI] Add "container.hot_path" tag to flag the hot path and inline related services

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:optim branch Nov 9, 2017

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