-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
b897e58
to
97dea49
Compare
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. |
c6ac7d7
to
bfd7d8f
Compare
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. |
bfd7d8f
to
15e4dad
Compare
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 |
121d2ed
to
5c4ecaf
Compare
There was a problem hiding this 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.
👍
5c4ecaf
to
d4db756
Compare
Thank you @jderusse. |
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:
I ran the container building script multiple times, but the results stayed consistent (within +-1 sec). |
Hello @kocsismate , thanks for reporting these numbers. You compared 2 differents version of the component (5.1 vs 5.2). From my side I tried with PHP 7.4, and got ~similar numbers. 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: The result are ~ identical because your project does not have many circular references, thus you were not impacted by the bug. |
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! |
… 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 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:
I also benched the PR with a project with few references and result are almost the same before/after.