Problem/Motivation

Recently we discovered a significant performance issue on a site we maintain. In our case, there were a combination of issues leading to 10+ second loads on a menu page. It turned out we had code that was making many tens of thousands of calls to load a config page value.

While we fixed the underlying issue, we also discovered that the \Drupal\config_pages\ConfigPagesLoaderService::load method doesn't do any caching of loaded entities.

Here's a call graph showing the path looping loading the same entity 10,000 times:

Call graph

Steps to reproduce

Benchmark code like:

for ($i = 0; $i < 10000; $i++) {
  \Drupal::service('config_pages.loader')->load('name_of_form');
}

Proposed resolution

Add or enable the built-in static cache for config pages. Possibly remove loadByProperties for calls where we load by a config page ID.

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

deviantintegral created an issue. See original summary.

heddn’s picture

I'm seeing similar performance issues on a site too. Going to see what can be done about it.

shumer made their first commit to this issue’s fork.

shumer’s picture

Assigned: Unassigned » shumer

shumer’s picture

Hey folks!
@heddn and @deviantintegral would you be able to try the code from the PR? It solves the issue for me.

The other question I have, what was the use case for the ConfigPages for both of you? I mean I can hardly imagine the situation like this, so I'm interested in what are you doing with this module? Hope that will help me to avoid issues like that in future releases.

Thx

shumer’s picture

Status: Active » Needs review
nicolas bouteille’s picture

Hello,
In the code of the PR 34, I see the $cache variable lives in the config() function.
Am I right to assume that the config page entity will be "cached" only during a single session ? Or maybe even a single request ? And that it only relieves the site when too many loads are done for a single request, but has no effect if multiple visitors load the config page in a short time?

In my website, I would like to check a config page's value on every single request made to the website.
The config page's value will not change frequently at all. So loading the config page entity on every page request or every session is overkill.
Which is why I am considering caching the data I need and removing it from cache whenever the config page is updated.
But I was wondering… do I really need to do this myself or config page / Drupal is already caching stuff?

So I stumbled upon this issue and it looks like no caching is done...
As mentioned above, no cache is made with loadByProperties() unlike load().
So as suggested above, shouldn't we use load() when context is null? Or is it because when context is null, we still need to fetch and specify the default context that we cannot do that?

BTW, on this documentation page : https://www.drupal.org/docs/contributed-modules/config-pages/usage-of-co...
The third option is to get a config page via storage manager:

$storage = \Drupal::entityTypeManager()->getStorage('config_pages');
$entity = $storage->load($config_page_machine_name);

this does not mention the context... so is the context not that important, or is the documentation incomplete?

If we cannot use load() instead of loadByProperties(), what do you think of caching the config page entity using cache_set() and remove it when it is updated? Or is it too heavy and we'd better only store simple values that we need to access frequently?

nicolas bouteille’s picture

Ok I have spotted that $storage->load() has been overridden in ConfigPagesStorage to actually call ConfigPages::config($id); unless the $id is numeric...
for that reason, calling $storage->load() inside ConfigPages::config would create an infinite loop
also I thought calling $storage->load('my_config_page') directly would be more efficient than using ConfigPagesLoaderService or ConfigPages::config because they use loadByProperties, but in the end all those three methods call loadByProperties in the end...

shumer’s picture

The way the entity is loaded here is not like how other entities in Drupal work. The reason for that is that we have Context enabled, so while the entity is being loaded, we need to evaluate which one exactly we need. We can't cache that enity between requests as the context might be different. In case of specific need you can cache the load in the custom code.

When it comes to a static cache we can add that option, but I'm still curious about the use case. I haven't seen anything like this before, despite I'm using that module since 2014... Can anybody from this topic provide me some info about the use case when this performance degradation appears?

shumer’s picture

Finally got some time to make a deeper look at the issue.

The bottleneck is the entity query inside loadByProperties() in ConfigPages::config(). Each call executes:

  1. ConfigPagesType::load($type) - already cached by core's memory cache, no issue here
  2. $type->getContextData() - computes context (invokes context plugins)
  3. loadByProperties(['type' => ..., 'context' => ...]) - runs an entity query (SELECT id FROM config_pages WHERE ...) every single time, then passes the found IDs to loadMultiple()
  4. Possibly a fallback query if the first one returns nothing

Core's loadMultiple() does use memory cache for the entity objects themselves - once entity ID 1 is loaded, subsequent load(1) calls hit the cache. But loadByProperties() always runs the query to find the ID first. So 10,000 calls to ConfigPages::config('my_type') means 10,000 identical SQL queries just to discover that the answer is entity ID 1.

Proposed solution in MR !34

The MR adds static $cache to the config() method. This works for the basic case but has issues:

  • Caches ConfigPagesType::load() results - unnecessary, core already handles this
  • No cache invalidation - if an entity is saved during the same request (e.g. form submit followed by rendering), the cache returns stale data
  • PHP static variables persist for the entire PHP process, which can cause subtle bugs in Drush commands, queue workers, and tests where multiple operations happen in a single process

Two better approaches

Approach A: Cache only the type+context to entity ID mapping

The core insight is that we only need to cache the lookup result - the mapping from (type, context) to entity_id. Once we have the ID, core's memory cache handles the rest via load($id).

public static function config($type = NULL, $context = NULL) {
  if (empty($type)) {
    return NULL;
  }

  static $idMap = [];

  $conditions['type'] = $type;
  if ($context === NULL) {
    $typeEntity = ConfigPagesType::load($type);
    if (!is_object($typeEntity)) {
      return NULL;
    }
    $conditions['context'] = $typeEntity->getContextData();
  }
  else {
    $conditions['context'] = $context;
  }

  $cid = $conditions['type'] . ':' . $conditions['context'];
  if (!isset($idMap[$cid])) {
    $list = \Drupal::entityTypeManager()
      ->getStorage('config_pages')
      ->loadByProperties($conditions);

    if (!$list && $context === NULL) {
      $conditions['context'] = $typeEntity->getContextData(TRUE);
      $cid = $conditions['type'] . ':' . $conditions['context'];
      $list = \Drupal::entityTypeManager()
        ->getStorage('config_pages')
        ->loadByProperties($conditions);
    }

    $entity = $list ? current($list) : NULL;
    $idMap[$cid] = $entity ? $entity->id() : NULL;
  }

  return $idMap[$cid] !== NULL
    ? \Drupal::entityTypeManager()
        ->getStorage('config_pages')
        ->load($idMap[$cid])
    : NULL;
}

Pros:

  • Simple, minimal change
  • The ID mapping is tiny (just integers), so memory footprint is negligible
  • The actual entity is always loaded through core's load($id) which uses memory cache with proper invalidation - if the entity is saved, resetCache() is called automatically by core, and the next load($id) returns fresh data
  • Eliminates 99% of the SQL queries in the reported scenario

Cons:

  • The ID mapping itself has no invalidation - if an entity is deleted and recreated with a different ID during the same request, the map points to a stale ID. This is an extremely unlikely scenario for config pages.
  • Still uses PHP static - persists for the entire process (affects Drush, tests, queue workers). For config pages this is acceptable since types and contexts rarely change mid-process.
  • getContextData() is still called on every cache miss per type. If context computation is expensive, this could be cached too, but it is typically cheap.

Approach B: Use core's entity.memory_cache service

Instead of a static variable, use Drupal's entity.memory_cache service (a MemoryCacheInterface backed by PHP array with cache tag support). Store the (type, context) -> entity_id mapping there and tag it with the entity's cache tag for automatic invalidation.

public static function config($type = NULL, $context = NULL) {
  if (empty($type)) {
    return NULL;
  }

  $conditions['type'] = $type;
  if ($context === NULL) {
    $typeEntity = ConfigPagesType::load($type);
    if (!is_object($typeEntity)) {
      return NULL;
    }
    $conditions['context'] = $typeEntity->getContextData();
  }
  else {
    $conditions['context'] = $context;
  }

  $cid = 'config_pages_lookup:' . $conditions['type']
    . ':' . $conditions['context'];
  $memoryCache = \Drupal::service('entity.memory_cache');
  $cached = $memoryCache->get($cid);

  if ($cached !== FALSE) {
    return $cached->data !== NULL
      ? \Drupal::entityTypeManager()
          ->getStorage('config_pages')
          ->load($cached->data)
      : NULL;
  }

  $list = \Drupal::entityTypeManager()
    ->getStorage('config_pages')
    ->loadByProperties($conditions);

  if (!$list && $context === NULL) {
    $conditions['context'] = $typeEntity->getContextData(TRUE);
    $cid = 'config_pages_lookup:' . $conditions['type']
      . ':' . $conditions['context'];
    $list = \Drupal::entityTypeManager()
      ->getStorage('config_pages')
      ->loadByProperties($conditions);
  }

  $entity = $list ? current($list) : NULL;
  $entityId = $entity ? $entity->id() : NULL;
  $tags = ['entity.memory_cache:config_pages'];
  $memoryCache->set($cid, $entityId,
    MemoryCacheInterface::CACHE_PERMANENT, $tags);

  return $entity;
}

Pros:

  • Automatic invalidation - when any config_pages entity is saved or deleted, core calls EntityStorageBase::resetCache() which invalidates the entity.memory_cache:config_pages tag, clearing our lookup cache
  • No stale data issues, even when entities are created/deleted during the same request
  • Follows core patterns - same cache backend that entity storage itself uses

Cons:

  • More complex implementation
  • Uses \Drupal::service() in a static method - not ideal for testability but consistent with how config() already works
  • Shares the LRU memory cache pool (1000 slots by default) with all entity types - in practice config pages will use very few slots so this is not a real concern
  • The MemoryCacheInterface service alias is deprecated in Drupal 11.3.0 (removed in 13.0.0)

I would say we can go with A option and migrate to B later if needed.

  • shumer committed f4021c41 on 8.x-2.x
    Resolve #3399222 "Cache option a"
    
shumer’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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