Add a cache for config page loads

Created on 4 November 2023, 8 months ago
Updated 14 June 2024, 16 days ago

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:

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

✨ Feature request
Status

Needs review

Version

2.0

Component

Code

Created by

πŸ‡¨πŸ‡¦Canada deviantintegral

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

Merge Requests

Comments & Activities

  • Issue created by @deviantintegral
  • heddn Nicaragua

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

  • First commit to issue fork.
  • Assigned to shumer
  • πŸ‡΅πŸ‡±Poland shumer Wroclaw
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update 6 months ago
    7 pass
  • πŸ‡΅πŸ‡±Poland shumer Wroclaw

    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

  • Status changed to Needs review 6 months ago
  • πŸ‡΅πŸ‡±Poland shumer Wroclaw
  • πŸ‡«πŸ‡·France Nicolas Bouteille

    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 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?

  • πŸ‡«πŸ‡·France Nicolas Bouteille

    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...

  • πŸ‡΅πŸ‡±Poland shumer Wroclaw

    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?

Production build 0.69.0 2024