Remove Drupal's Container::get()

I do agree that pure xhprof based benchmarking is not the right tool for the job
and ab still can show if there is a difference.

In order to understand it on the miro level I did some small benchmark: http://nbviewer.ipython.org/gist/dawehner/78c9f19cefdeaae5c9e4
The clear result is that on the micro level this is quite an improvement factor 3. This effect will be much less in reality because you have more than one service in the container.

The failure seems to be a bug in our container and the set() method.

Just to clarify, ab should not be used for benchmarking like this. It introduces far too many unrelated variables.

When xhprof's overhead is enough to influence the results, like it is here, microtime is the tool to use. Just throw two microtime(true)'s in index.php and write the results to a csv file.

It's fine to use ab to generate traffic, but don't use the time that it took to serve the full request as your measurement.

On top of that we can also optimize the is_object call itself, which itself saves you a couple of calls, but also wall time itself. (50 nodes on the frontpage, standard install)

=== 8.0.x..2307869-1 compared (544a67cf8ad3f..544a6be5184a9):                                                                            │
                                                                                                                                         │
ct  : 77680|75444|-2236|-2.9%                                                                                                            │
wt  : 423201|417254|-5947|-1.4%                                                                                                          │
cpu : 418208|413734|-4474|-1.1%                                                                                                          │
mu  : 23594424|23580848|-13576|-0.1%                                                                                                     │
pmu : 23673520|23659264|-14256|-0.1%                                                                                                     │
                                                                                                                                         │
-i used with no filenames on the command line, reading from STDIN.                                                                       │
<a href="http://d8.dev/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=544a67cf8ad3f&run2=544a6be5184a9&extra=8.0│
.x..2307869-1">Profiler output</a>                                                                                                       │
                                                                                                                                         │
---                                                                                                                                      │
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage         

Measuring wall time results in considerable overhead, especially for method calls. We should measure this without that or possibly even do micro-benchmarks with manual microtime() comparisons.

For any micro/benchmarks I think we should setup a scenario on 3v4l.org:

http://3v4l.org/8BRYI/perf vs. http://3v4l.org/GHpAH/perf

We can see that calling the parent is still 50% slower in PHP 5.5 for example.

There have been other services that provide performance comparisons for PHP, but I don't know them.

But yes setup scenarios for microbenchmarks yourself is way better.

On the other hand if we can simply remove one function call, that is a clear yes - if its called enough times and xhprof can tell us that.

I went with a standard D8 frontpage: 1,217 calls in total to Container::get, with the appended script I found the following difference, for the given patch:

Before

Benching function Array
Array
(
    [attempts] => 1
    [internal offset] => 0.00029736518859863
    [manual offset] => 0
    [covar] => 7.5623847092704E-6
    [stdev] => 2.0586622326846E-8
    [raw mean] => 0.0027222394943237
    [internally adjusted mean] => 0.0024248743057251
    [fully adjusted mean] => 0.0024248743057251
)

After

Benching function Array
Array
(
    [attempts] => 1
    [internal offset] => 0.00032463312149048
    [manual offset] => 0
    [covar] => 2.9162324849065E-6
    [stdev] => 2.1894493329455E-9
    [raw mean] => 0.00075078010559082
    [internally adjusted mean] => 0.00042614698410034
    [fully adjusted mean] => 0.00042614698410034
)

According to that, we indeed even save 2ms for that really simple site.
Note: 100 will dissapear alone from getting rid of hook_library_alter() per library but rathter one in total.

Note: This is with disabled xdebug/uprofiler.

Given that result, this seems to be at least major.

Major +1.

+++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
@@ -18,9 +18,12 @@ class Container extends SymfonyContainer {
-    if (is_object($service)) {
+    if ($service === (object) $service) {

Given that the parent can only return an object or NULL, wouldn't a strict check for NULL be faster than this type check/assignment?

+++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
@@ -18,9 +18,12 @@ class Container extends SymfonyContainer {
     $service = parent::get($id, $invalidBehavior);

If we're really down to micro-optimizing Drupal\Core\DependencyInjection\Container::get() calls and an extra stack call is such a problem (which I doubt), we can always inline the parent method's code in order to remove at least that call. Nevemind this, the first hunk of the patch achieves the same quite nicely. @effulgentsia++

Benchmarking is hard :) Your script isn't that fair because you always call it with the same argument, so you more or less only test the first check, which means it is obviously a lot faster.

I changed it to 10k calls to see a bit more differences and tested a version without that:

HEAD:
Array
(
    [attempts] => 1
    [internal offset] => 0.00097194910049438
    [manual offset] => 0
    [covar] => 9.9728565679393E-6
    [stdev] => 1.1068450248786E-7
    [raw mean] => 0.011098575592041
    [internally adjusted mean] => 0.010126626491547
    [fully adjusted mean] => 0.010126626491547
)

Patch:
Array
(
    [attempts] => 1
    [internal offset] => 0.00097799062728882
    [manual offset] => 0
    [covar] => 3.9061859623751E-6
    [stdev] => 1.3947537619667E-8
    [raw mean] => 0.0035706281661987
    [internally adjusted mean] => 0.0025926375389099
    [fully adjusted mean] => 0.0025926375389099
)

Patch without the $this->services check:
Array
(
    [attempts] => 1
    [internal offset] => 0.00098129749298096
    [manual offset] => 0
    [covar] => 4.166603248372E-5
    [stdev] => 4.2633758766897E-7
    [raw mean] => 0.010232257843018
    [internally adjusted mean] => 0.0092509603500366
    [fully adjusted mean] => 0.0092509603500366
)

So the (object) thing seems to be slightly faster but not by much and the results vary between ~0.0092 and ~0.01, the tries stuff doesn't seem to really work, I increased it to 1000 and the results varied just as much.

Also, results from of 3v4l:
$object === (object) $object:
http://3v4l.org/dEQYC/perf#tabs

$object !== NULL:
http://3v4l.org/Hf8vq/perf#tabs

So the (object) thing is at least as fast and on some versions faster.. results seem pretty stable. A bit surprised by this :)

It wouldn't matter, because neither !== NULL nor just doing a (if ($service = parent::get()) seem to work for, I get two Attempt to assign property of non-object Container.php:24 with both versions and the microbench script and I can *not* figure out why.

Anyway, the only relevant improvement seems to be the $this->services check, and for that, it depends a lot on how many times we call the same service.. the more the more we can save ;)

Hm, tried to fix the failing tests, but there are two problems:

a) Because the unit test calls set() with the test service, we already populate $this->services, we just return that object.
b) This also means that synthetic services that are always manually set would not be detected anymore by the dependency serialization trait?

I can think of two ways to fix this:

1. See attached patch. no longer immediately return but add an isset(), at least in xhprof, that seems to be almost as fast. And 2.5 times as fast in the microbenchmark.

2. Also set the _serviceId in set(). Didn't try that yet. That would also "fix" the unit test, but we wouldn't be testing the same as before. The only way I can think of to still test get() would be to simulate a compiled container by subclassing and manually adding a getTestService() method.

Thoughts?

Here's another idea: we could try to push the DependencySerializationTrait solution upstream and get rid of our Container class entirely.

This overridden container was one of points from @fabpot's "list of stuff he doesn't like in the Drupal-Symfony integration" that he talked about at the Commerce 2.x sprint a few months ago, so a solution for this in Symfony 2.7 seems like a win for everyone?

We had good experience with symfony latetly but especially DependencySerializationTrait::__wakup() can't be backported because they don't
provide a static available container.

Here though is an alternative idea: Let's just add the service ID onto the dumped container.

After this patch the container looks like the following:

    /**
     * Gets the 'language_manager' service.
     *
     * This service is shared.
     * This method always returns the same instance of the service.
     *
     * @return \Drupal\Core\Language\LanguageManager A Drupal\Core\Language\LanguageManager instance.
     */
    protected function getLanguageManagerService()
    {
        $this->services['language_manager'] = $instance = new \Drupal\Core\Language\LanguageManager($this->get('language.default'));

        $instance->_serviceId = 'language_manager';

        return $instance;
    }

    /**
     * Gets the 'library.discovery' service.
     *
     * This service is shared.
     * This method always returns the same instance of the service.
     *
     * @return \Drupal\Core\Asset\LibraryDiscovery A Drupal\Core\Asset\LibraryDiscovery instance.
     */
    protected function getLibrary_DiscoveryService()
    {
        $a = $this->get('module_handler');

        $b = new \Drupal\Core\Asset\LibraryDiscoveryParser($this->get('app.root'), $a);
        $b->_serviceId = 'library.discovery.parser';

        $c = new \Drupal\Core\Asset\LibraryDiscoveryCollector($this->get('cache.discovery'), $this->get('lock'), $b);
        $c->_serviceId = 'library.discovery.collector';

        $this->services['library.discovery'] = $instance = new \Drupal\Core\Asset\LibraryDiscovery($c, $a);

        $instance->_serviceId = 'library.discovery';

        return $instance;
    }

As you see, also public: false services are supported.

Ha, that was a good teaser :P

Mh, so it turns out, that we have with the app.root string a problem, so let's check before we set the property.

1. +++ b/core/core.services.yml
   @@ -390,6 +390,9 @@ services:
   +    is_class: false
   

   Is that a Symfony-specific thing? Otherwise I don't see this being used in the patch.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
   @@ -22,7 +22,9 @@ class DependencySerializationTraitPass implements CompilerPassInterface {
   +      if (!$definition->hasTag('non_class')) {
   +        $definition->setProperty('_serviceId', $service_id);
   +      }
   

   It is unfortunate that Symfony does not check something like is_object() itself before setting properties on a service. Or are Symfony services always objects?

Is that a Symfony-specific thing? Otherwise I don't see this being used in the patch.

Oh right, we should remove that, this was just an experiment I made.

It is unfortunate that Symfony does not check something like is_object() itself before setting properties on a service. Or are Symfony services always objects?

At least according to ContainerInterface::get() they are. One alternative way I tried out was to introduce a AppRoot class which just stores the string,
and at the same time, implements ::__toString().

At least according to ContainerInterface::get() they are. One alternative way I tried out was to introduce a AppRoot class which just stores the string,
and at the same time, implements ::__toString().

Ahh, I see. Thanks. That's not for this issue then, so that's fine for now.

We have to be aware of the fact that we can't just unserialize private services ...

I'm not sure what exactly were are testing in the unit test now? Can we unit test the compiler pass class instead and check for all variants there?

I think this needs another round of profiling, not sure if the microbench script is still useful or if we should just do an xhprof check of a full page, but I assume this is as fast as it can get, nice work!

Ah, as discussed in IRC, what I think we should overwrite now is set(), for synthetic services. Surprised that we're not seeing any failures but I guess they are usually not directly injected in objects that get serialized.

According to people in #symfony IRC, synthetic services are not useful for anything beside the kernel, class_loader and service_container. This was different before request was removed from the container. From that discussion I got the impression that synthetic services are going away rather sooner than later. Some people even thought that synthetic services were already deprecated, and nobody actually could think of a use-case other than the ones named before. Rather than add support for synthetic services in corner cases, I believe we better should deprecate them right away in Drupal.

Yes, and how would you want to replace those three?

It doesn't matter if there's just one and/or they are deprecated. If you, for some reason, inject the kernel into a class that gets serialized, it *will* break.

Yes, and how would you want to replace those three?

Special case them, like we do with the container already (see Container::__sleep()).

Ah, as discussed in IRC, what I think we should overwrite now is set(), for synthetic services. Surprised that we're not seeing any failures but I guess they are usually not directly injected in objects that get serialized.

Valid point.

Alright.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
@@ -0,0 +1,34 @@
+      // Some services might have strings internally.
+      // Given that you can just reload a service which is accessible via
+      // Container::get, you need to filter out public services here.

just = only?

Trying to rewrite this a bit...

"Only add the property to services that are public (as private services can not be reloaded through Container::get()) and are objects." ?

As mentioned in #2380293: Properly inject services into ModuleInstaller, if we want to implement set() like we currently do, then we can remove the custom properties that are set over there.

"Only add the property to services that are public (as private services can not be reloaded through Container::get()) and are objects." ?

+1

Shouldn't we focus on #2389193: Provide a way to determine whether a given instance was retrieved from the container instead? That looks like a better solution to me.

@amateescu: I'm not sure that is really a better option.

Here's a reroll with an improved comment.

I think the only thing is the issue referenced in #38.

I would prefer to get this in anyway, this is definitely an improvement and I'm not sure if the other issue really will be or if it will work, there are still a lot of test fails to figure out AFAIK.

+++ b/core/core.services.yml
@@ -442,6 +442,8 @@ services:
     class: SplString
     factory_service: 'app.root.factory'
     factory_method: 'get'
+    tags:
+      - { name: non_class }

class: SplSptring... tag is non_class. I think the tag could be better named - perhaps something about the effect that it actually has.

We went with this more generic name as there might be more cases where it matters in the future.

@alexpott, I have added the tag as mentioned below and as SplString is a class has been assigned to name.

app.root:
    class: SplString
    factory_service: 'app.root.factory'
    factory_method: 'get'
    tags:
      - { name: SplString, tag: 'non_class'}

Adding interdiff also.

Showing Drupal installation failed after adding.

 tags:
      - { name: SplString, tag: 'non_class'}

So I have reverted it to previous stag.

class: SplSptring... tag is non_class. I think the tag could be better named - perhaps something about the effect that it actually has.

What about mutable_parameter or parameter_service ?

Let's use parameter_service. Note: This patch starts from #39

Meh.

RTBC as per #59.

This issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 9cf83e9 and pushed to 8.0.x. Thanks!