From aaf6426b64e6c51f2b3c35e7d4d8b8c3665531ad Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Fri, 23 Dec 2022 15:25:14 +1000 Subject: [PATCH] Issue #3327856 by alexpott, catch, zcht, longwave, znerol, Berdir, BramDriesen, Spokje, callen321, andypost, neclimdul, acbramley, mstrelan: Performance regression introduced by container serialization solution --- core/core.services.yml | 2 + .../ContainerInterface.php | 10 ++ .../DependencyInjection/ReverseContainer.php | 98 +++++++++++++++++++ .../ServiceIdHashTrait.php | 7 +- .../DependencySerializationTrait.php | 38 +++---- core/lib/Drupal/Core/DrupalKernel.php | 27 ++++- .../lib/Drupal/Core/DrupalKernelInterface.php | 5 + core/lib/Drupal/Core/Test/TestKernel.php | 2 + .../tests/src/Kernel/DecoratedServiceTest.php | 9 +- .../DependencyInjection/ContainerTest.php | 7 +- .../ReverseContainerTest.php | 58 +++++++++++ .../DependencySerializationTest.php | 2 + .../Core/DrupalKernel/DrupalKernelTest.php | 3 + 13 files changed, 233 insertions(+), 35 deletions(-) create mode 100644 core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php create mode 100644 core/tests/Drupal/Tests/Component/DependencyInjection/ReverseContainerTest.php diff --git a/core/core.services.yml b/core/core.services.yml index 2891d06ceba..6655001af19 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -386,6 +386,8 @@ services: class: Drupal\Core\Plugin\Context\LazyContextRepository arguments: ['@service_container'] Drupal\Core\Plugin\Context\ContextRepositoryInterface: '@context.repository' + Drupal\Component\DependencyInjection\ReverseContainer: + arguments: [ '@service_container' ] cron: class: Drupal\Core\Cron arguments: ['@module_handler', '@lock', '@queue', '@state', '@account_switcher', '@logger.channel.cron', '@plugin.manager.queue_worker', '@datetime.time'] diff --git a/core/lib/Drupal/Component/DependencyInjection/ContainerInterface.php b/core/lib/Drupal/Component/DependencyInjection/ContainerInterface.php index e6c5ea72d45..47567c5cdf8 100644 --- a/core/lib/Drupal/Component/DependencyInjection/ContainerInterface.php +++ b/core/lib/Drupal/Component/DependencyInjection/ContainerInterface.php @@ -25,6 +25,11 @@ interface ContainerInterface extends BaseContainerInterface { * * @return array * Service ids keyed by a unique hash. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ public function getServiceIdMappings(): array; @@ -36,6 +41,11 @@ interface ContainerInterface extends BaseContainerInterface { * * @return string * A unique hash identifying the object. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ public function generateServiceIdHash(object $object): string; diff --git a/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php b/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php new file mode 100644 index 00000000000..3678cb4f087 --- /dev/null +++ b/core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php @@ -0,0 +1,98 @@ +getServiceId = \Closure::bind(function ($service): ?string { + /** @phpstan-ignore-next-line */ + return array_search($service, $this->services, TRUE) ?: NULL; + }, $serviceContainer, $serviceContainer); + } + + /** + * Returns the ID of the passed object when it exists as a service. + * + * To be reversible, services need to be public. + * + * @param object $service + * The service to find the ID for. + */ + public function getId(object $service): ?string { + if ($this->serviceContainer === $service || $service instanceof SymfonyContainerInterface) { + return 'service_container'; + } + + $hash = $this->generateServiceIdHash($service); + $id = self::$recordedServices[$hash] ?? ($this->getServiceId)($service); + + if ($id !== NULL && $this->serviceContainer->has($id)) { + self::$recordedServices[$hash] = $id; + return $id; + } + + return NULL; + } + + /** + * Records a map of the container's services. + * + * This method is used so that stale services can be serialized after a + * container has been re-initialized. + */ + public function recordContainer(): void { + $service_recorder = \Closure::bind(function () : array { + /** @phpstan-ignore-next-line */ + return $this->services; + }, $this->serviceContainer, $this->serviceContainer); + self::$recordedServices = array_merge(self::$recordedServices, array_flip(array_map([$this, 'generateServiceIdHash'], $service_recorder()))); + } + + /** + * Generates an identifier for a service based on the object class and hash. + * + * @param object $object + * The object to generate an identifier for. + * + * @return string + * The object's class and hash concatenated together. + */ + private function generateServiceIdHash(object $object): string { + // Include class name as an additional namespace for the hash since + // spl_object_hash's return can be recycled. This still is not a 100% + // guarantee to be unique but makes collisions incredibly difficult and even + // then the interface would be preserved. + // @see https://php.net/spl_object_hash#refsect1-function.spl-object-hash-notes + return get_class($object) . spl_object_hash($object); + } + +} diff --git a/core/lib/Drupal/Component/DependencyInjection/ServiceIdHashTrait.php b/core/lib/Drupal/Component/DependencyInjection/ServiceIdHashTrait.php index d53f9cb650d..fda1ece21a4 100644 --- a/core/lib/Drupal/Component/DependencyInjection/ServiceIdHashTrait.php +++ b/core/lib/Drupal/Component/DependencyInjection/ServiceIdHashTrait.php @@ -5,7 +5,10 @@ namespace Drupal\Component\DependencyInjection; /** * A trait for service id hashing implementations. * - * Handles delayed cache tag invalidations. + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ trait ServiceIdHashTrait { @@ -13,6 +16,7 @@ trait ServiceIdHashTrait { * Implements \Drupal\Component\DependencyInjection\ContainerInterface::getServiceIdMappings() */ public function getServiceIdMappings(): array { + @trigger_error(__METHOD__ . "() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942", E_USER_DEPRECATED); $mapping = []; foreach ($this->getServiceIds() as $service_id) { if ($this->initialized($service_id) && $service_id !== 'service_container') { @@ -26,6 +30,7 @@ trait ServiceIdHashTrait { * Implements \Drupal\Component\DependencyInjection\ContainerInterface::generateServiceIdHash() */ public function generateServiceIdHash(object $object): string { + @trigger_error(__METHOD__ . "() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942", E_USER_DEPRECATED); // Include class name as an additional namespace for the hash since // spl_object_hash's return can be recycled. This still is not a 100% // guarantee to be unique but makes collisions incredibly difficult and even diff --git a/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php index a23a2f487ab..65501b07474 100644 --- a/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php @@ -2,9 +2,9 @@ namespace Drupal\Core\DependencyInjection; +use Drupal\Component\DependencyInjection\ReverseContainer; use Drupal\Core\Entity\EntityStorageInterface; -use Symfony\Component\DependencyInjection\ContainerInterface as SymfonyContainerInterface; -use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException; +use Drupal\Core\StringTranslation\TranslatableMarkup; /** * Provides dependency injection friendly methods for serialization. @@ -34,8 +34,12 @@ trait DependencySerializationTrait { $vars = get_object_vars($this); try { $container = \Drupal::getContainer(); - $mapping = \Drupal::service('kernel')->getServiceIdMapping(); + $reverse_container = $container->get(ReverseContainer::class); foreach ($vars as $key => $value) { + if (!is_object($value) || $value instanceof TranslatableMarkup) { + // Ignore properties that cannot be services. + continue; + } if ($value instanceof EntityStorageInterface) { // If a class member is an entity storage, only store the entity type // ID the storage is for, so it can be used to get a fresh object on @@ -47,34 +51,18 @@ trait DependencySerializationTrait { $this->_entityStorages[$key] = $value->getEntityTypeId(); unset($vars[$key]); } - elseif (is_object($value)) { - $service_id = FALSE; - // Special case the container. - if ($value instanceof SymfonyContainerInterface) { - $service_id = 'service_container'; - } - else { - $id = $container->generateServiceIdHash($value); - if (isset($mapping[$id])) { - $service_id = $mapping[$id]; - } - } - if ($service_id) { - // If a class member was instantiated by the dependency injection - // container, only store its ID so it can be used to get a fresh object - // on unserialization. - $this->_serviceIds[$key] = $service_id; - unset($vars[$key]); - } + elseif ($service_id = $reverse_container->getId($value)) { + // If a class member was instantiated by the dependency injection + // container, only store its ID so it can be used to get a fresh object + // on unserialization. + $this->_serviceIds[$key] = $service_id; + unset($vars[$key]); } } } catch (ContainerNotInitializedException $e) { // No container, no problem. } - catch (ServiceNotFoundException $e) { - // No kernel, very strange, but still no problem. - } return array_keys($vars); } diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php index 59a41d31d3b..c2457761e06 100644 --- a/core/lib/Drupal/Core/DrupalKernel.php +++ b/core/lib/Drupal/Core/DrupalKernel.php @@ -10,6 +10,7 @@ use Drupal\Core\Cache\DatabaseBackend; use Drupal\Core\Config\BootstrapConfigStorageFactory; use Drupal\Core\Config\NullStorage; use Drupal\Core\DependencyInjection\ContainerBuilder; +use Drupal\Component\DependencyInjection\ReverseContainer; use Drupal\Core\DependencyInjection\ServiceModifierInterface; use Drupal\Core\DependencyInjection\ServiceProviderInterface; use Drupal\Core\DependencyInjection\YamlFileLoader; @@ -238,6 +239,11 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { /** * A mapping from service classes to service IDs. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ protected $serviceIdMapping = []; @@ -795,8 +801,14 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { * * @return string * A unique hash value. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ public static function generateServiceIdHash($object) { + @trigger_error(__METHOD__ . "() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942", E_USER_DEPRECATED); // Include class name as an additional namespace for the hash since // spl_object_hash's return can be recycled. This still is not a 100% // guarantee to be unique but makes collisions incredibly difficult and even @@ -809,6 +821,7 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { * {@inheritdoc} */ public function getServiceIdMapping() { + @trigger_error(__METHOD__ . "() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942", E_USER_DEPRECATED); $this->collectServiceIdMapping(); return $this->serviceIdMapping; } @@ -858,8 +871,12 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { if ($this->container->initialized('current_user')) { $current_user_id = $this->container->get('current_user')->id(); } - // Save the current services. - $this->collectServiceIdMapping(); + // After rebuilding the container some objects will have stale services. + // Record a map of objects to service IDs prior to rebuilding the + // container in order to ensure + // \Drupal\Core\DependencyInjection\DependencySerializationTrait works as + // expected. + $this->container->get(ReverseContainer::class)->recordContainer(); // If there is a session, close and save it. if ($this->container->initialized('session')) { @@ -1574,8 +1591,14 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { /** * Collect a mapping between service to ids. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ protected function collectServiceIdMapping() { + @trigger_error(__METHOD__ . "() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942", E_USER_DEPRECATED); if (isset($this->container)) { foreach ($this->container->getServiceIdMappings() as $hash => $service_id) { $this->serviceIdMapping[$hash] = $service_id; diff --git a/core/lib/Drupal/Core/DrupalKernelInterface.php b/core/lib/Drupal/Core/DrupalKernelInterface.php index 0378dff3e7d..dfd00f20170 100644 --- a/core/lib/Drupal/Core/DrupalKernelInterface.php +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php @@ -139,6 +139,11 @@ interface DrupalKernelInterface extends HttpKernelInterface, ContainerAwareInter /** * Get a mapping from service hashes to service IDs. + * + * @deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the + * 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. + * + * @see https://www.drupal.org/node/3327942 */ public function getServiceIdMapping(); diff --git a/core/lib/Drupal/Core/Test/TestKernel.php b/core/lib/Drupal/Core/Test/TestKernel.php index 855d695fac2..c1f64d1b038 100644 --- a/core/lib/Drupal/Core/Test/TestKernel.php +++ b/core/lib/Drupal/Core/Test/TestKernel.php @@ -3,6 +3,7 @@ namespace Drupal\Core\Test; use Drupal\Core\DependencyInjection\ContainerBuilder; +use Drupal\Component\DependencyInjection\ReverseContainer; use Drupal\Core\DrupalKernel; /** @@ -38,6 +39,7 @@ class TestKernel extends DrupalKernel { // inside those objects. $kernel->container = $container; $container->set('kernel', $kernel); + $container->set(ReverseContainer::class, new ReverseContainer($container)); \Drupal::setContainer($container); return $container; } diff --git a/core/modules/system/tests/src/Kernel/DecoratedServiceTest.php b/core/modules/system/tests/src/Kernel/DecoratedServiceTest.php index 099019da517..f1bc1bd88eb 100644 --- a/core/modules/system/tests/src/Kernel/DecoratedServiceTest.php +++ b/core/modules/system/tests/src/Kernel/DecoratedServiceTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\system\Kernel; +use Drupal\Component\DependencyInjection\ReverseContainer; use Drupal\decorated_service_test\TestServiceDecorator; use Drupal\KernelTests\KernelTestBase; @@ -22,16 +23,12 @@ class DecoratedServiceTest extends KernelTestBase { public function testDecoratedServiceId() { // Service decorated once. $test_service = $this->container->get('test_service'); - $hash = $this->container->generateServiceIdHash($test_service); - $mappings = $this->container->getServiceIdMappings(); - $this->assertEquals('test_service', $mappings[$hash]); + $this->assertEquals('test_service', $this->container->get(ReverseContainer::class)->getId($test_service)); $this->assertInstanceOf(TestServiceDecorator::class, $test_service); // Service decorated twice. $test_service2 = $this->container->get('test_service2'); - $hash = $this->container->generateServiceIdHash($test_service2); - $mappings = $this->container->getServiceIdMappings(); - $this->assertEquals('test_service2', $mappings[$hash]); + $this->assertEquals('test_service2', $this->container->get(ReverseContainer::class)->getId($test_service2)); $this->assertInstanceOf(TestServiceDecorator::class, $test_service2); } diff --git a/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php b/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php index b0f81565b2f..46d5dcc75bb 100644 --- a/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php @@ -10,6 +10,7 @@ namespace Drupal\Tests\Component\DependencyInjection; use Drupal\Component\Utility\Crypt; use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException; use Symfony\Component\DependencyInjection\Exception\LogicException; @@ -24,7 +25,7 @@ use Prophecy\Argument; * @group DependencyInjection */ class ContainerTest extends TestCase { - + use ExpectDeprecationTrait; use ProphecyTrait; /** @@ -688,8 +689,12 @@ class ContainerTest extends TestCase { /** * @covers \Drupal\Component\DependencyInjection\ServiceIdHashTrait::getServiceIdMappings * @covers \Drupal\Component\DependencyInjection\ServiceIdHashTrait::generateServiceIdHash + * + * @group legacy */ public function testGetServiceIdMappings() { + $this->expectDeprecation("Drupal\Component\DependencyInjection\ServiceIdHashTrait::generateServiceIdHash() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942"); + $this->expectDeprecation("Drupal\Component\DependencyInjection\ServiceIdHashTrait::getServiceIdMappings() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942"); $this->assertEquals([], $this->container->getServiceIdMappings()); $s1 = $this->container->get('other.service'); $s2 = $this->container->get('late.service'); diff --git a/core/tests/Drupal/Tests/Component/DependencyInjection/ReverseContainerTest.php b/core/tests/Drupal/Tests/Component/DependencyInjection/ReverseContainerTest.php new file mode 100644 index 00000000000..8fa73191650 --- /dev/null +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/ReverseContainerTest.php @@ -0,0 +1,58 @@ +set('bar', $service); + + $reverse_container = new ReverseContainer($container); + + $this->assertSame('bar', $reverse_container->getId($service)); + $non_service = new \stdClass(); + $this->assertNull($reverse_container->getId($non_service)); + $this->assertSame('service_container', $reverse_container->getId($container)); + } + + /** + * @covers ::recordContainer + */ + public function testRecordContainer(): void { + $container = new ContainerBuilder(); + $service = new \stdClass(); + $container->set('bar', $service); + + $reverse_container = new ReverseContainer($container); + $reverse_container->recordContainer(); + + $container = new ContainerBuilder(); + $reverse_container = new ReverseContainer($container); + + // New container does not have a bar service. + $this->assertNull($reverse_container->getId($service)); + + // Add the bar service to make the lookup based on the old object work as + // expected. + $container->set('bar', new \stdClass()); + $this->assertSame('bar', $reverse_container->getId($service)); + } + +} diff --git a/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php index 22f4d53a96f..0f660717ca7 100644 --- a/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php @@ -7,6 +7,7 @@ namespace Drupal\Tests\Core\DependencyInjection; +use Drupal\Component\DependencyInjection\ReverseContainer; use Drupal\Core\DependencyInjection\DependencySerializationTrait; use Drupal\Core\Test\TestKernel; use Drupal\Tests\UnitTestCase; @@ -37,6 +38,7 @@ class DependencySerializationTest extends UnitTestCase { /** @var \Drupal\Tests\Core\DependencyInjection\DependencySerializationTestDummy $dependencySerialization */ $dependencySerialization = unserialize($string); + $this->assertTrue($container->has(ReverseContainer::class)); $this->assertSame($service, $dependencySerialization->service); $this->assertSame($container, $dependencySerialization->container); $this->assertEmpty($dependencySerialization->getServiceIds()); diff --git a/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php b/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php index b94cf7ec05c..16b5b0d4757 100644 --- a/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php +++ b/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php @@ -140,8 +140,11 @@ EOD; /** * @covers ::getServiceIdMapping + * @group legacy */ public function testGetServiceIdMapping() { + $this->expectDeprecation("Drupal\Core\DrupalKernel::getServiceIdMapping() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942"); + $this->expectDeprecation("Drupal\Core\DrupalKernel::collectServiceIdMapping() is deprecated in drupal:9.5.1 and is removed from drupal:11.0.0. Use the 'Drupal\Component\DependencyInjection\ReverseContainer' service instead. See https://www.drupal.org/node/3327942"); $service = new BarClass(); $container = TestKernel::setContainerWithKernel(); $container->set('bar', $service);