10000 Use best options for Symfony, fix Cobalt+Joomla prototype containers by nicolas-grekas · Pull Request #21 · kocsismate/php-di-container-benchmarks · GitHub
[go: up one dir, main page]
Skip to content

Use best options for Symfony, fix Cobalt+Joomla prototype containers #21

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 12, 2018
Merged

Use best options for Symfony, fix Cobalt+Joomla prototype containers #21

merged 1 commit into from
Feb 12, 2018

Conversation

nicolas-grekas
Copy link
Contributor
@nicolas-grekas nicolas-grekas commented Feb 12, 2018 •
edited
Loading
  • update to Symfony 4.0.4
  • fixes Joomla & Cobalt container in prototype mode (they were just non-shared or empty before)
  • turn on as_files and inline_class_loader options, that's the default for skeleton Symfony 4 apps

@kocsismate
Copy link
Owner

Awesome! I couldn't configure the file-based container myself properly when I tried to play with it so thank you very much for the help! I'll rerun the test suite soon.

@kocsismate kocsismate merged commit 07ac12f into kocsismate:master Feb 12, 2018
@nicolas-grekas nicolas-grekas deleted the symfony-tuned branch February 12, 2018 15:10
@theofidry
Copy link
Contributor

ping us when you do, I think a lot of people will be excited to see the results as a lot of work has been done there :)

@kocsismate
Copy link
Owner
kocsismate commented Feb 12, 2018 •
edited
Loading

Having to restart my computer and shut down all the apps in order to improve the accuracy is blocking the re-run. :D But I'll be able to do that in an hour! :)

@kocsismate
Copy link
Owner

@nicolas-grekas, @theofidry, I've just generated the results (many-many times, even with different setups), but unfortunately Symfony seems to be a bit slower now. :( (though the difference is usually negligible)

I am a bit disappointed because I expected - and wanted to see - big performance improvements. How much difference do you have on your systems? What did Blackfire say?

Although I am not very familiar with the internals of autoloading, I wonder if it tricks us. I think so because the biggest change was that Composer's autoloader is replaced with include_once invocations. So my very wild guess is that either my tests don't measure autoloading time properly or maybe opcache does something very differently than what you reckoned on. What do you think?

I would be happy if you had a good explanation about this weird phenomenon. :/ :)

@nicolas-grekas
Copy link
Contributor Author

Haha :)
On my local setup, I identified 3 separate situations:

  • on cold tests, Symfony was usually 1st or 2nd
  • on non-shared (prototype) tests, Symfony generates too many include_once, fixed in 4.1
  • on warmed up tests, it cannot be fastest, because it does 2 more isset checks (which IMHO means these tests are mostly non representative, we're not at 2 isset checks to decide which DI container to use.)

@theofidry
Copy link
Contributor

on warmed up tests, it cannot be fastest, because it does 2 more isset checks (which IMHO means these tests are mostly non representative, we're not at 2 isset checks to decide which DI container to use.)

True and the benchmark is not lying: the difference between rank 7 (Symfony) and rank 1 (Zen) for those is around .2ms...

Also compared to before (3.4), the results are significantly better :)

@nicolas-grekas
Copy link
Contributor Author

For reference, if we make this change, Symfony comes always first in the 3rd ranking group where it ranks 6th/7th for now:

--- a/src/Symfony/Component/DependencyInjection/Container.php
+++ b/src/Symfony/Component/DependencyInjection/Container.php
@@ -217,6 +217,11 @@ class Container implements ResettableContainerInterface
      */
     public function get($id, $invalidBehavior = /* self::EXCEPTION_ON_INVALID_REFERENCE */ 1)
     {
+        return $this->services[$id] ?? ($this->factories[$id] ?? array($this, 'make'))($id, $invalidBehavior);
+    }
+
+    private function make($id, $invalidBehavior)
+    {
         if (isset($this->aliases[$id])) {
             $id = $this->aliases[$id];
         }

@kocsismate
Copy link
Owner

I also agree that the last two test suites are the least useful and I hardly ever pay attention to them. I'll only leave them there because still, they might help someone who wants to optimize $container->get() calls. But maybe the number of iterations could be increased because as you also noticed, the difference is too small between containers.

Initially, my main source of confusion was that Symfony is not much faster in the first 2 suites. But Nicolas is right, the test suite 3 and 4 are much more affected by the changes. And when I talked about the "autoloader's trick", I suspected that the include_onces are needlessly called all the time. So thanks Nicolas for reassuring this (and I've also found your commit: symfony/dependency-injection@e615852).

I happily regenerate the results with Symfony 4.1 if you want :) I am also curious!

@kocsismate
Copy link
Owner
kocsismate commented Feb 13, 2018 •
edited
Loading

With Symfony 4.0.4:

symfony 4 0

With Symfony 4.1:

symfony 4 1

@nicolas-grekas Big congratulations :) This is fascinating!

@nicolas-grekas nicolas-grekas mentioned this pull request Feb 13, 2018
Merged
@nicolas-grekas
Copy link
Contributor Author

And Symfony might come always 1st within the error margin if symfony/symfony#26161 is merged :)

@kocsismate
Copy link
Owner

I'll keep an eye on the PR and will publish the updated results as soon as I can :)

chalasr added a commit to symfony/symfony that referenced this pull request Feb 13, 2018
@chalasr
minor #26161 [DI] Top micro benchmarks (nicolas-grekas)
4d6c481 
This PR was merged into the 4.1-dev branch.

Discussion
----------

[DI] Top micro benchmarks

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

This makes Symfony rank 1st most of the time, 2nd within the margin error otherwise.
Follow up of kocsismate/php-di-container-benchmarks#21

Commits
-------

3edf5f1 [DI] Top micro benchmarks
@nicolas-grekas
Copy link
Contributor Author

Now merged :)
Should I send a PR to upgrade to 4.1? Can you do it?

@kocsismate
Copy link
Owner

No need for a PR, I'll do that now!

@kocsismate
Copy link
Owner

Done! Symfony is really on the top for almost all the tests :)

@nicolas-grekas
Copy link
Contributor Author

Let's celebrate :)
https://twitter.com/nicolasgrekas/status/963365262840954880

@theofidry
Copy link
Contributor

🎉

A small note regarding the results. As you can see below, sometimes the ranking doesn't make sense, maybe it would be good to adjust the second ranking criteria:

screen shot 2018-02-13 at 11 50 59

In some cases, the difference is incredibly small (0.009ms = 20%) and this for up to rank 6 sometimes. I would suggest to for example calculate a delta and for the the rank 1 + delta add a color indicating that the results are sensibly the same. For example:

Rank Container Time (ms) Time (%) Peak Memory (MB) Peak Memory (%)
1 Yaco 0.01 100% 0.649 100%
2 PHP-DI 0.011 110% 0.627 97%
3 Aura 0.011 110% 0.841 130%
4 Symfony 0.011 110% 0.619 95%
5 Zen 0.011 110% 0.679 105%
6 Zend ServiceManager 0.011 110% 0.713 110%
7 Yii2 Container 0.012 120% 0.768 118%
8 Dice 0.013 130% 0.855 132%
9 Pimple 0.019 190% 0.727 112%
10 Cobalt 0.023 230% 0.757 117%
11 Joomla 0.025 250% 0.747 115%
12 Opulence IoC 0.031 310% 0.729 112%
13 Laravel 0.039 390% 0.695 107%
14 Disco 0.04 400% 0.807 124%
15 PHPixie DI 0.045 450% 0.705 109%
16 Auryn 0.064 640% 0.795 123%

For above from rank 1 to rank 8, the difference is so small they should kind of all appear as "super good"

@kocsismate
Copy link
Owner
kocsismate commented Feb 13, 2018 •
edited
Loading

@theofidry Great idea! But what would you think about if I increased the number of iterations in the last 2 test suites? They will become even more synthetic this way, but I guess it is not a problem in this case.

@nicolas-grekas I couldn't resist the temptation of trying to optimize Zen... After many attempts in the past, I finally found a "nice" way to eliminate some opcodes in the $container->get() method with the following change: https://github.com/woohoolabs/zen/compare/4976acd6a27264616f01cec99658c99ec1f1d91f...46753cce657f3136e15997ae4aaefcc2b8946046?diff=unified&name=46753cce657f3136e15997ae4aaefcc2b8946046#diff-f348ecacd0d17522912d6b2f2b91c1bbR27

So Zen is the first now in the last 2 test suites! But I'll be happy if you can squeeze out even more speed from Symfony and it comes first again! A little competition helps us to do our best :)

Update: I've just realized by checking your last optimization again (but more carefully now) that essentially I did the same as you: optimized the ifs with ??. Initially, I thought that your change only creates a shot-circuit for non-aliased services. Ah, I even was a bit proud that I could find such a nasty way to throw an exception from the ?? check... :D

@nicolas-grekas
Copy link
Contributor Author
nicolas-grekas commented Feb 14, 2018 •
edited
Loading

Haha how unexpected.
There is nothing else to squeeze on Symfony side, at some moment we have some features that are measurable in a microbench. ;)

@mbabker mbabker mentioned this pull request Mar 23, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolas-grekas @kocsismate @theofidry
0