Problem/Motivation

Symfony 3.4 introduced some performance improvements to \Symfony\Component\DependencyInjection\Container and in #2927746: Update Symfony components to 3.4.* we've upgraded to 3.4 but we did the minimum changes necessary to make it work. We should look at the changes and improvements in 3.4 and see if we can apply to Drupal.

Proposed resolution

Remaining tasks

User interface changes

None

API changes

to be decided - should be none.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

neclimdul’s picture

Status: Postponed » Needs review
FileSize
2.55 KB

I found this rummaging in ContainerBuilder. I'm a little confused about why some methods where left in the previous issue. Though it was probably just that a bunch of patches where being worked on so a final review wasn't done after they all landed.

The methods ContainerBuilder::callMethod and ContainerBuilder::shareService are both private in Symfony. #2927746: Update Symfony components to 3.4.* removed the last calls to them inside Drupal which means the only calls to them that can be made are in Symfony. But since symfony's versions of those methods are private so the code on the Drupal container is just dead and not doing anything. Best I can tell we're already using those optimizations... maybe by accident?

Anyways, this patch cleans up the dead code in ContainerBuilder. I'm also removing getProxyInstantiator because its private and the last call to it was remove in #2726645: Do not @deprecate EntityManager until all usages are removed (helps the Symfony 3 update).

Just skimming the file, to my eyes the // @codingStandardsIgnoreFile can probably be removed as well because we're not copying any Symfony code anymore.

Maybe there's additional optimizations I'm missing but we can get this rolling I think.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

longwave’s picture

longwave’s picture

FileSize
2.78 KB

So I think this is probably OK to remove. Even if someone extends ContainerBuilder (and they don't in contrib; http://grep.xnddx.ru/search?text=extends+ContainerBuilder&filename=) would they call these methods?

I also agree that @codingStandardsIgnoreFile can be removed now, as I think can "@todo Submit upstream patches to Symfony to not require these overrides" as there is seemingly nothing useful left for upstream now - I've removed both those in this patch.

longwave’s picture

FileSize
3.09 KB

Fixed coding standards issue.

alexpott’s picture

Our says \Drupal\Core\DependencyInjection\ContainerBuilder::set()

   * @todo Restrict this to synthetic services only. Ideally, the upstream
   *   ContainerBuilder class should be fixed to allow setting synthetic
   *   services in a frozen builder.

It appears this restriction has been fixed in the 4.4 ContainerBuilder so maybe we can call the parent here rather than the grandparent...

longwave’s picture

Title: Bring \Drupal\Core\DependencyInjection\ContainerBuilder inline with \Symfony\Component\DependencyInjection\Container and apply 3.4 improvements » Bring ContainerBuilder inline with Symfony Container and apply upstream improvements
FileSize
3.88 KB
1.68 KB

Ah yeah it does indeed look like the @todo is fixed upstream.

longwave’s picture

Reuploading #10, as that failed spectacularly.

The issue is this code in Symfony\Component\DependencyInjection\ContainerBuilder::get():

        unset($this->definitions[$id]);

I don't understand enough about the container build process but somehow this then results in an exception when we compile the container:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "cache_tags.invalidator.checksum" has a dependency on a non-existent service "database".

/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:86
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:82
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:49
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:91
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:49
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:82
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:49
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/AbstractRecursivePass.php:46
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php:40
/var/www/html/drupal/vendor/symfony/dependency-injection/Compiler/Compiler.php:94
/var/www/html/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php:762
/var/www/html/drupal/core/lib/Drupal/Core/DrupalKernel.php:1303
/var/www/html/drupal/core/lib/Drupal/Core/DrupalKernel.php:908
/var/www/html/drupal/core/lib/Drupal/Core/DrupalKernel.php:473
/var/www/html/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:370
/var/www/html/drupal/core/tests/Drupal/KernelTests/KernelTestBase.php:248
alexpott’s picture

#12 is due to

diff --git a/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php b/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
index b139bb71d53..b31a9fa65d5 100644
--- a/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
@@ -49,7 +49,8 @@ public function process(ContainerBuilder $container) {
       try {
         $driver_backend = $container->get('database')->driver();
         $default_backend = $container->get('database')->databaseType();
-        $container->set('database', NULL);
+        // @todo this does not look right.
+        // $container->set('database', NULL);
       }
       catch (\Exception $e) {
         // If Drupal is not installed or a test doesn't define database there

Doing $container->set('database', NULL); would now unset it. In our container it'll cause the service to be rebuilt. So this is another difference that at some point we're going to address. Even if we do fix this we'll then hit the problem that we have persistent, non-synthetic services which will cause:

In ContainerBuilder.php line 502:

  Setting service "request_stack" for an unknown or non-synthetic service definition on a compiled container is not allowed.

during a site install and probably during a module install too.

So I guess punting this further down the track is okay.

alexpott’s picture

Here's the issue to fix our odd persist thing... #2368263: Remove the persist service tag

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Triggered a retest against 9.3.x.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost’s picture

andypost’s picture

Ghost of Drupal Past’s picture

#14

neclimdul tracked down what happens there with very little help from me. #2531564: Fix leaky and brittle container serialization solution will fix it.

What an incredible, incredible mess.

_serviceId is set on a service object using the property injection facility. DependencySerializationTraitPass sets this up (this was the easy part I contributed). If a compiler pass runs before that and creates a service then that service instance will not have _serviceId and sleeping it might break (this is the hard part neclimdul figured out).

The two issues are hopelessly intertwined though because allowing set on the compiled container breaks that solution as I noted in #216.

So shall we merge these two? The two issues form a circular dependency: you can't kill set without killing _serviceId as shown in #14 here and you can't kill _serviceId without killing set as shown in #216 over there.

It's possible though that I wanted to post this on #3300306: Drupal ContainerBuilder allows mutation of non-synthetic services.

How many heads does this hydra have?

longwave’s picture

Personal opinion is that we should commit #22 here as that is easy cleanup, and then we can solve #23 in a separate issue where the patch will be more focused and slightly smaller.

Ghost of Drupal Past’s picture

I agree, I commented here because the code snippet we wrestled with today was posted in #14 here but I think the decision was to punt it so this might not be the place for this.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Let's get #22 in and continue in the other issues.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 9cd6d55883 to 10.1.x and 2c5031a5c0 to 10.0.x and 00b239dc20 to 9.5.x. Thanks!

  • alexpott committed 9cd6d55 on 10.1.x
    Issue #2937010 by longwave, andypost, neclimdul, alexpott, Charlie ChX...

  • alexpott committed 2c5031a on 10.0.x
    Issue #2937010 by longwave, andypost, neclimdul, alexpott, Charlie ChX...

  • alexpott committed 00b239d on 9.5.x
    Issue #2937010 by longwave, andypost, neclimdul, alexpott, Charlie ChX...
andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.