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] Optimize Container::get() for perf #25474

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
@nicolas-grekas
Copy link
Member

commented Dec 12, 2017

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

As outlined in #25159, our beloved container suffers from a perf hit just because it has a second argument, and this second argument's default value makes it slower.

Let's fix this by inlining the value. This will put Symfony at a better rank on eg:
https://github.com/kocsismate/php-di-container-benchmarks

I benched also with the following script. The result is surprising (but matches the finding in #25159):
without the patch, it takes 2s, and with the patch, it's down to 1s (opcache enabled).


require './vendor/autoload.php';

use Symfony\Component\DependencyInjection\Container;

$c = new Container();
$c->set('foo', new \stdClass());

$i = 10000000;

$s = microtime(1);

while (--$i) {
    $c->get('foo');
}

echo microtime(true) - $s, "\n";

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 12, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-inline-default branch from 401a8c0 to b3386c6 Dec 12, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-inline-default branch from b3386c6 to 4fe2551 Dec 12, 2017

@fabpot

fabpot approved these changes Dec 12, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 4fe2551 into symfony:3.4 Dec 12, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Dec 12, 2017

bug #25474 [DI] Optimize Container::get() for perf (nicolas-grekas)
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Optimize Container::get() for perf

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

As outlined in #25159, our beloved container suffers from a perf hit just because it has a second argument, and this second argument's default value makes it slower.

Let's fix this by inlining the value. This will put Symfony at a better rank on eg:
https://github.com/kocsismate/php-di-container-benchmarks

I benched also with the following script. The result is surprising (but matches the finding in #25159):
without the patch, it takes 2s, and with the patch, it's down to 1s (opcache enabled).

```php

require './vendor/autoload.php';

use Symfony\Component\DependencyInjection\Container;

$c = new Container();
$c->set('foo', new \stdClass());

$i = 10000000;

$s = microtime(1);

while (--$i) {
    $c->get('foo');
}

echo microtime(true) - $s, "\n";
```

Commits
-------

4fe2551 [DI] Optimize Container::get() for perf

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-inline-default branch Dec 13, 2017

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

This is just an idea that came up reading this PR, but would doing this for the dumped code be a similar easy fix?

@Koc

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

Oh, why PHP's opcode does not inline constants values? Is it possible to do something here from PHP side? // @nikic

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@dkarlovi I think we already do that and not only for the container but for our own code too. Example: #24866

@stof

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@Koc because the constant is defined in a separate file, and OPCache optimizations are per-file (to avoid having the optimized state depending on the list of previously loaded files)

@zilionis

This comment has been minimized.

Copy link

commented Dec 13, 2017

One stupid question. Why you chose 10 000 000 times, but not 1 000 000 000 (or whatever). So it can be 1000% or 100000% faster. For me this micro optimization is useless. We just lose code quality here.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

@zilionis really? Can you run the script locally with these different numbers and report back?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@zilionis I think there's some error in those numbers. If Nico's benchmarks iterate 10 million times and goes from 2s to 1s ... if we increase iterations 10x to 100 million times, I expect time to go from 20s to 10s ... so the gain remains at 100% and it doesn't increase.

In any case, we can of course be wrong, so please share your script so we can check the numbers again. Thanks!

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

@zilionis and for the second part of your comment, as replied on Twitter (https://twitter.com/nicolasgrekas/status/940885273587707904)

yes, it matters when this code is run by a large community that expect the framework to be as seamless as possible even in tight loops.

@zilionis

This comment has been minimized.

Copy link

commented Dec 13, 2017

@nicolas-grekas @javiereguiluz ok, I am feeling stupid now. But anyway but about this micro change 100% looked to big for me. So i made benchmark (same test script). And I got just 15-25% performance increase.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@zilionis I was curious so I run the benchmarks myself (I'm using PHP 7.1). These are the numbers:

BEFORE 3.4896380901337
AFTER  1.6525621414185

So in my case it's consistent with 100% performance increase.

@mvrhov

This comment has been minimized.

Copy link

commented Dec 13, 2017

Sorry, but I find this stupid, now we have a freaking magic number.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

(PHP 7.2 btw here, didn't try on anything else recently)

@zilionis

This comment has been minimized.

Copy link

commented Dec 13, 2017

Currently running on work computer (7.0)
before 9.3259789943695
after 8.2541401386261
12.99%

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@mvrhov "stupid" and "freaking" words are a bit strong. Please let's keep discussions friendly :)

And yes, you are right that this goes against the recommended practices but:

  • It provides a huge performance improvement
  • We add a useful comment next to the "magic number", so it's impossible to mistake
@stof

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

and we do it only for the code belonging to the hot path of the container, not for all places using the constants (all code dealing with compile-time logic has no reason to inline the value)

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

because the constant is defined in a separate file, and OPCache optimizations are per-file

It seems like this specific change might benefit more complex codebases greatly, it makes sense to try and resolve the constant even if the file containing it is not yet loaded.

@zilionis

This comment has been minimized.

Copy link

commented Dec 13, 2017

7.2
➜ my_project php test.php
1.7662029266357
0.92619895935059
90.69%

IMHO 7.0 vs 7.2 (5-10 times faster!)

@Koc

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

@stof thank you for explanation. No way to copy this constants to class, something like:

const EXCEPTION_ON_INVALID_REFERENCE = ContinerInterface::EXCEPTION_ON_INVALID_REFERENCE;

?

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

Is this really hot path for Symfony 4+? There isn't lot of public services nowadays.

@stof

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@Koc the fact that you reference the constant from the interface has the same issue: OPCache could maybe inline the reference to the constant defined in the current file, replacing it by a direct reference to ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE (i.e. our old code). but it still does not have the value of ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE in this file (as ContainerInterface is in a different file)

@curry684

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

Well, given that we already compile the container into an non-humanly readable mess in a generated namespace, why wouldn't we inline all of it? The compiler can look up the final value of those constants, so we can completely inline all of it. Ie.

class appLocalDebugProjectContainer extends Container
{
    const EXCEPTION_ON_INVALID_REFERENCE = 1;
    const NULL_ON_INVALID_REFERENCE = 2;
    const IGNORE_ON_INVALID_REFERENCE = 3;
    const IGNORE_ON_UNINITIALIZED_REFERENCE = 4;
   ...

Alternatively, PHP is fine with multiple namespaces in the same file, so we could even put the entire inheritance tree of the container into the compiled container file so they're all in the same opcache scope. Might get messy if the autoloader got one of them first though.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2017

Everyone is coming with better ideas, but please run them before, the script is so simple.
Here is what your suggestion triggers:

Fatal error: Cannot inherit previously-inherited or override constant EXCEPTION_ON_INVALID_REFERENCE from interface Symfony\Component\DependencyInjection\ResettableContainerInterface in [...]

@curry684

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

Oh I was already expecting that not to work but given stof's answer I was blindly assuming I was wrong in this haha.

This was referenced Dec 15, 2017

@kocsismate kocsismate referenced this pull request Jan 24, 2018

Merged

Upgrade to Symfony 4 #20

@kelunik

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

Constants from the same class should already be inlined by PHP: https://3v4l.org/jM5BA/vld#output.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2018

@kelunik not when taking inheritance into account (and the parent class is in another file.)

@stof

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

@kelunik these constants are not from the same class but from a parent

@emirb

This comment has been minimized.

Copy link

commented May 3, 2018

Class constants are now inlined at runtime.
Implemented in master: php/php-src@1a63fa6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.