Implement static caching around SchedulerManager::getPlugins()

Created on 2 July 2025, 25 days ago

Problem/Motivation

I've been doing some Blackfire profiling lately and noticed SchedulerManager::getPlugins() being called 8 times in an add to cart request (via a JSONAPI endpoint).

This means 8 requests to Redis (which might not sound a lot), but it is unnecessary as we could simply retrieve the plugins once per request, and statically cache them in an array.

See the attached screenshot, won't be able to work on this today, but maybe in the upcoming days if the maintainers are interested.

Feature request
Status

Active

Version

2.0

Component

Code

Created by

🇮🇱Israel jsacksick

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jsacksick
  • 🇮🇱Israel jsacksick

    Currently, when a provider is passed, the persistent cache is bypassed. With this change, we always use the static cache if present, then fallback to the persistent cache. The provider filtering is done after the fact. I think this makes more sense than bypassing the persistent cache entirely when a $provider is passed.

  • Pipeline finished with Failed
    24 days ago
    Total: 308s
    #537849
  • Pipeline finished with Failed
    24 days ago
    Total: 401s
    #537852
  • Pipeline finished with Failed
    24 days ago
    Total: 356s
    #537856
  • Pipeline finished with Failed
    24 days ago
    Total: 268s
    #537865
  • Pipeline finished with Failed
    24 days ago
    Total: 803s
    #537869
  • Pipeline finished with Success
    24 days ago
    Total: 624s
    #537892
  • 🇮🇱Israel jsacksick

    Attaching a static patch for use with composer.

  • 🇬🇧United Kingdom jonathan1055

    This looks interesting, thanks for working on it.
    I've just merged 🐛 Unhandled Warning: Undefined array key "translatable" Active and would like to tag a new release soon, so maybe we can get this improvement in too?

  • 🇮🇱Israel jsacksick

    I think this is an interesting fix, the getPlugins() method is called a lot and currently each call to that method will make a request to the cache backend (in my case Redis), For example, 8 cache get calls on a simple add to cart request.

    I'm actually wondering if the persistent cache fully makes sense here, because doGetPlugins() calls $this->getPluginDefinitions();Which gets the definitions from the plugin manager:

    $this->pluginManager->getDefinitions();

    This invokes the getDefinitions() method on the DefaultPluginManager which has the following logic:

    /**
       * {@inheritdoc}
       */
      public function getDefinitions() {
        $definitions = $this->getCachedDefinitions();
        if (!isset($definitions)) {
          $definitions = $this->findDefinitions();
          $this->setCachedDefinitions($definitions);
        }
        return $definitions;
      }

    So the plugin definitions already have static cache + persistent cache handling. So we define our own cache on top of the core cache handling.

    Our own persistent cache probably still makes sense otherwise we'll perform the following logic over and over:

        $definitions = $this->getPluginDefinitions();
        foreach ($definitions as $definition) {
          $plugin = $this->pluginManager->createInstance($definition['id']);
          $dependency = $plugin->dependency();
          // Ignore plugins if there is a dependency module and it is not enabled.
          if ($dependency && !$this->moduleHandler->moduleExists($dependency)) {
            continue;
          }
          $this->plugins[$plugin->entityType()] = $plugin;
        }
    
  • 🇬🇧United Kingdom jonathan1055

    I notice you have fixed a dependency injection, but we have 📌 Fix \Drupal calls should be avoided in classes, use dependency injection Active already, so I'd prefer if we keep this issue in scope. Was there a particular need for that to be fixed here? If not I will undo that, otherwise there will be a conflict.

  • Pipeline finished with Failed
    12 days ago
    Total: 540s
    #547796
  • 🇬🇧United Kingdom jonathan1055

    I've merged in 2.x and resolved the conflict, so there's nothing more to do regarding #8.

  • Pipeline finished with Skipped
    12 days ago
    #547843
  • 🇬🇧United Kingdom jonathan1055

    Merged. Thank you very much for doing this work, it is a good improvement.

Production build 0.71.5 2024