Contrib.social

Performance regression introduced by container serialization solution

Open on Drupal.org →
Created on 18 December 2022, almost 2 years ago
Updated 9 March 2023, over 1 year ago

Problem/Motivation

#2531564: Fix leaky and brittle container serialization solution → changes the way in which services are re-injected into objects. Unfortunately the new approach can result in performance issues because we're working out the mapping all the time.

There is some suspicion that this is responsible for a performance regression between 9.4 and 9.5 as reported by @Chris Allen...

Per the conversation above ... I did some profiling locally for both 9.4.9 and 9.5.0. The goal was to dissect uncached responses. The totals are averages from 3 runs of each test case. The call stack is ordered by descending wall time and taken from just one run that I thought described the average order.
The biggest thing that stands out to me is the time spent running Container::getServiceIdMappings and Container::generateServiceIdHash. Also, the number of calls to DefaultPluginManager::cacheSet went up on the cold start.
https://hackmd.io/@chris-allen/ByWLzLoOo

Steps to reproduce

Run the script:

$a = \Drupal\Core\Url::fromRoute('system.cron_settings');
$b = \Drupal\Core\Url::fromRoute('system.admin');
serialize($a);
serialize($b);

This will result in rebuilding the entire service hash map twice because there is no saving of the work done in \Drupal\Core\DrupalKernel::collectServiceIdMapping().

Proposed resolution

Inspired by \Symfony\Component\DependencyInjection\ReverseContainer we have our own ReverseContainer service that performs a similar service for the Drupal containers.

Remaining tasks

User interface changes

API changes

The methods are depercated:

  • \Drupal\Component\DependencyInjection\ContainerInterface::getServiceIdMappings()
  • \Drupal\Component\DependencyInjection\ContainerInterface::generateServiceIdHash()
  • \Drupal\Core\DrupalKernel::collectServiceIdMapping()
  • \Drupal\Core\DrupalKernel::generateServiceIdHash()
  • \Drupal\Core\DrupalKernelInterface::getServiceIdMapping()

\Drupal\Component\DependencyInjection\ServiceIdHashTrait is completely deprecated too.

Data model changes

None

Release notes snippet

I'm not sure one is worth it. The deprecated code really has no use-case outside of core and was new to 9.5 so very very unlikely to be used anywhere anyway.

🐛 Bug report
Status

Fixed

Version

9.5

Component
Base  →

Last updated about 1 hour ago

Created by

🇬🇧United Kingdom alexpott 🇪🇺🌍

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇺🇸United States tr Cascadia

    The change record and the recommendations in there was still using

    Drupal\Component\DependencyInjection\ReverseContainer

    as the class name instead of the proper (changed in #66)

    Drupal\Component\DependencyInjection\ReverseContainer

    I have modified the change record to fix this.

    Dear Core Committers and Reviewers,
    In the future please try to do a better job verifying change records before marking core issues as RTBC and before committing core changes, as mistakes like this, which seem pretty frequent to me, have an impact on contrib developers.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024