Skip to content

[DependencyInjection] fix performances in circular refs detection #38882

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

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Oct 29, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37850
License MIT
Doc PR -

This PR change the way circular references are detected. And improve the project submitted in #37850 by 86% (Build the container in 1.1 sec instead of 10)

Issue is: When a project contains a lot Circular Reference, the Dumper spend a lot of time in merging those circular references.
note: a circular reference is not an issue when an service is not injected by constructor, but this Can not be known until all references are resolved (performed previously by connectCircularReferences)

This PR removed the connectCircularReferences and generate a flatten tree of dependencies:

  • the key is the service ID
  • the value is the list of direct AND indirect dependency + path to join the dependency

I also benched the PR with a project with few references and result are almost the same before/after.

@ostrolucky
Copy link
Contributor

I don't want to spam original issue regarding testing results of this PR. I've just retested this with the zip I attached in issue, I can confirm promising results: getting 4s with a patch and ~49s without this patch.

I've also retested my normal project from which I extracted the demo, there time didn't change much (which makes sense, I already replaced injections of these problematic services with container injections there). This time I used your link to 4.4 file, perhaps during my previous testing I applied patch in this PR manually wrong and that resulted in some infinite loop.

@jderusse jderusse changed the base branch from 5.x to 4.4 October 29, 2020 21:41
@jderusse jderusse force-pushed the perf-circular-52 branch 3 times, most recently from c6ac7d7 to bfd7d8f Compare October 29, 2020 22:19
@jderusse
Copy link
Member Author

Thank you @ostrolucky for your tests.

This PR changes a lot of things in the way the container is Dumped, The output of the circularReferences before and after the PR are not comparable but the dumped container is identical in the cases I tested.

I think we should test this PR is several different configuration to be sure.

@jderusse
Copy link
Member Author

Rework a little bit the implementation with the help of @nicolas-grekas

Now reduced by 88% CPU time for the same memory usage https://blackfire.io/profiles/compare/ba4c464a-4449-4633-99a8-fcf31bcc948c/graph

@jderusse jderusse force-pushed the perf-circular-52 branch 4 times, most recently from 121d2ed to 5c4ecaf Compare October 30, 2020 18:41
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really cool, thanks for having a deep look at this. Despite being labeled as a performance improvement, this really is a bug fix and should be for 4.4. The current logic follows too many paths and can take millions of iterations to complete.
The new logic doesn't inspect all possible loops, but it inspects the ones that matter.

👍

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Improve performances in CircualReference detection [DependencyInjection] fix performances in CircualReference detection Nov 2, 2020
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] fix performances in CircualReference detection [DependencyInjection] fix performances in circular refs detection Nov 2, 2020
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 8375eee into symfony:4.4 Nov 2, 2020
@jderusse jderusse deleted the perf-circular-52 branch November 4, 2020 17:45
@kocsismate
Copy link

Just a heads up, I've built the Symfony containers for https://github.com/kocsismate/php-di-container-benchmarks via PHP 8, running on Docker on my laptop:

  • v5.1.8: 11805.0532 ms
  • 5.x-dev: 14068.2973 ms

I ran the container building script multiple times, but the results stayed consistent (within +-1 sec).

@jderusse
Copy link
Member Author

jderusse commented Nov 5, 2020

Hello @kocsismate , thanks for reporting these numbers.

You compared 2 differents version of the component (5.1 vs 5.2).
Could you please compare with version v5.2.0-BETA3

From my side I tried with PHP 7.4, and got ~similar numbers.
5.2.0-BETA3: 1928.255282 ms
5.2-dev: 1995.516162 ms

Note, I also extract number from the part that have been optimized (dumping the php code)

diff --git a/src/Container/Symfony/SymfonyContainerAdapter.php b/src/Container/Symfony/SymfonyContainerAdapter.php
index 4f7d174..69f4921 100644
--- a/src/Container/Symfony/SymfonyContainerAdapter.php
+++ b/src/Container/Symfony/SymfonyContainerAdapter.php
@@ -139,6 +139,8 @@ final class SymfonyContainerAdapter implements ContainerAdapterInterface
     protected function dumpContainer(ContainerBuilder $containerBuilder, string $path, string $class, bool $asFiles): void
     {
         $dumper = new PhpDumper($containerBuilder);
+
+        $s = microtime(true);
         $content = $dumper->dump(
             [
                 "namespace" => "DiContainerBenchmarks\\Container\\Symfony\\Resource",
@@ -147,6 +149,7 @@ final class SymfonyContainerAdapter implements ContainerAdapterInterface
                 "debug" => false,
             ]
         );
+        var_dump(microtime(true) - $s);
 
         if ($asFiles) {
             $file = key($content);

and get very similar number:
5.2.0-BETA3: float(0.035419940948486) => 354 ms
5.2-dev: float(0.037868022918701) => 378 ms

The result are ~ identical because your project does not have many circular references, thus you were not impacted by the bug.

@kocsismate
Copy link

Yeah, I can confirm, 5.2.0-BETA3 and 5.2-dev yields nearly the same results. Thanks for the explanation about the difference between my initial two measurements!

@fabpot fabpot mentioned this pull request Nov 10, 2020
derrabus added a commit that referenced this pull request Nov 12, 2020
… paths (jderusse)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Fix circular detection with multiple paths

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39056
| License       | MIT
| Doc PR        | -

There are currently 2 kind of issues related to the Dependency Injection:

1. performance issue when project contains many loops (#37850)
Which has been fixed by #38882

2. Infinity loop in some case (#38970)
Which has been fixed by #38980 and #39021

The new issue #39056 has been introduced by #38882 (The performance issue refactor) because in order to optimize loop detection, I take a short cut and choose to not collect ALL the circular loop but only the one that matters

I was wrong. All loops matters.

This PR fix my previous refacto to collect ALL the paths, with a low CPU footprint

Commits
-------

1c3721e Fix circular detection with multiple paths
This was referenced Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Injecting certain services causes O(n) compilation complexity, while injecting Container is O(1)
5 participants