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

Suggestion to increase performance of $container->get(), $container->has() and $container->make() #531

Closed
holtkamp opened this Issue Oct 30, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@holtkamp
Copy link
Contributor

holtkamp commented Oct 30, 2017

When in the process of increasing performance of our application, I noticed two things:

  • fetched Definitions are not (locally) cached, for instance in a local array property $acquiredDefinitions[$name] => $definition
  • array_key_exists() is used, while isset() tends to perform better, although:
    • it is considered a micro-optimization...
    • when the value equals NULL it will be considered 'not set'

What do you think? Would a PR on this make sense? It can be as easy as:

private function getDefinition($name)
{
    if(!array_key_exists($name, $this->acquiredDefinitions)){
        $this->acquiredDefinitions[$name] = $this->definitionSource->getDefinition($name);
    }

    return  $this->acquiredDefinitions[$name];
}

Maybe $lookedUpDefinitions or $fetchedDefinitions is a better name for the property...

Attached a part of a BlackFire.io profiling session of the current situation.

screen shot 2017-10-30 at 13 44 36

Adding this local cache reduces the amount of lookups with around 390 calls.

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Oct 30, 2017

Hi, thanks for all those details and the pull request. Have you tried an alpha release of the future v6? It is meant to be much faster (the container is compiled), so I'm not sure this optimisation is really necessary.

@holtkamp

This comment has been minimized.

Copy link
Contributor

holtkamp commented Oct 31, 2017

Have you tried an alpha release of the future v6?

Yeah, tried it a while back, but back then the migration effort was too high + v5 is quite stable and "works for me" 😄

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Oct 31, 2017

Thanks that's valuable feedback, do you remember what was causing the effort? I can probably add a few helpers or maybe documentation in the v6 release to help others that are in your situation.

Anyway regarding v5 I guess we could merge your pull request in a new v5 release but not port it to v6 as to avoid complicating the code for no gain?

@holtkamp

This comment has been minimized.

Copy link
Contributor

holtkamp commented Oct 31, 2017

Thanks that's valuable feedback, do you remember what was causing the effort?

Mmm, don't remember, maybe I can revive this PHP-DI-6 feature-branch of our project somewhere this week.

Anyway regarding v5 I guess we could merge your pull request in a new v5 release but not port it to v6 as to avoid complicating the code for no gain?

Totally agree, will add the missing code as pointed out in #533 (comment)

@holtkamp

This comment has been minimized.

Copy link
Contributor

holtkamp commented Oct 31, 2017

Thanks that's valuable feedback, do you remember what was causing the effort?

Gave 6.0.0 a new shot, the following issue showed up:

So if the suggested performance improvement can be merged for 5.4 that would be nice in the mean time 😄

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Nov 2, 2017

Thanks for the issues!

So looking back on the problem and I'm not sure I understand:

  • in v5 definitions are cached ($containerBuilder->setDefinitionCache(...)) so the cache you want to add is doing the same job
  • the blackfire graph you are showing focuses on resolving definitions, which is not the same thing as fetching them (from a definition source), is that on purpose?

@mnapoli mnapoli added the enhancement label Nov 2, 2017

@holtkamp

This comment has been minimized.

Copy link
Contributor

holtkamp commented Nov 3, 2017

in v5 definitions are cached ($containerBuilder->setDefinitionCache(...)) so the cache you want to add is doing the same job

well, I am not sure about that. Resolving a $definitionX to a $valueOrInstanceX is cached. But given an entry entryX, which points to $definitionX, that process (let's name that "fetching the definition") is not cached.

So suppose I got a definition like:

[
    'translator' => \My\Project\I18n\Translator::class
]

When processing an incoming request, this translator is injected quite a lot (in templates, etc). Let's say 50 times, then the definition is "fetched" / looked up 50 times as well using $this->definitionSource->getDefinition($name). Not a "huge" performance drop, but still an easy pick to improve using the suggested approach.

The same for this example:

$object = $container->has('definitionName') ? $container->get('definitionName') : null;

This would involve invoking $this->definitionSource->getDefinition($name) two times, which is/seems not efficient...

the blackfire graph you are showing focuses on resolving definitions, which is not the same thing as fetching them (from a definition source), is that on purpose?

Well, it was to point out the amount of resolving that is done. It did not yet include the suggested DI\Container::getDefinition(). In the following stats you can see that the suggested DI\Container::getDefinition() is hit around 4 times more than the DI\Definition\Source\CachedDefinitionSource::getDefinition.

screen shot 2017-11-03 at 01 03 25

Note that these are just my 2 cents trying to get max performance and reduce having code performing the same task twice 🤓. When looking at the code for v6, it seems the same performance gain can be achieved. It seems the PerformanceTests focusses on running a lot of requests instead of resolving a lot of dependencies in the same request, maybe some looping can be done to resolve the same dependency multiple times in the same run/request. I have little experience with PHPBench, but this might be a good excuse to dive into this 😄

@holtkamp

This comment has been minimized.

Copy link
Contributor

holtkamp commented Nov 5, 2017

Ok, added a PHPBench benchmark in af40f9d, the difference is subtle, but measurable 😄. It seems the make() operation benefits most.

Without local cache

+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+
| benchmark      | subject                     | groups | params | revs   | iter | mem_peak   | time_rev | comp_z_value | comp_deviation |
+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+
| ContainerBench | benchGetDummyClass          |        | []     | 100000 | 0    | 1,628,344b | 0.319μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchCheckAndGetDummyClass  |        | []     | 100000 | 0    | 1,628,392b | 0.531μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchMakeDummyClass         |        | []     | 100000 | 0    | 1,628,344b | 9.106μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchCheckAndMakeDummyClass |        | []     | 100000 | 0    | 1,628,392b | 11.683μs | 0.00σ        | 0.00%          |
+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+

With local cache

+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+
| benchmark      | subject                     | groups | params | revs   | iter | mem_peak   | time_rev | comp_z_value | comp_deviation |
+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+
| ContainerBench | benchGetDummyClass          |        | []     | 100000 | 0    | 1,628,344b | 0.336μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchCheckAndGetDummyClass  |        | []     | 100000 | 0    | 1,628,392b | 0.558μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchMakeDummyClass         |        | []     | 100000 | 0    | 1,628,344b | 7.212μs  | 0.00σ        | 0.00%          |
| ContainerBench | benchCheckAndMakeDummyClass |        | []     | 100000 | 0    | 1,628,392b | 8.692μs  | 0.00σ        | 0.00%          |
+----------------+-----------------------------+--------+--------+--------+------+------------+----------+--------------+----------------+
@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Nov 5, 2017

OK I had a chat yesterday with somebody else which pointed in the same direction so maybe it could be a good addition. Also it could be useful for v6 also because make() is not compiled yet.

@mnapoli mnapoli closed this Nov 11, 2017

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