Merging cache metadata is slow with thousands of tags added.

Created on 19 October 2023, about 1 year ago

Problem/Motivation

The situation where I see merging taking a long time is huge GraphQL requests (thousands of entities using https://www.drupal.org/project/graphql ).

When GraphQL resolves fields they can add cache metadata to the object that holds metadata for the whole request, and a lot of tags can be added multiple times.
Every call to addCacheTags performs merging of these tags, so if there are already a few thousand tags each added pack of tags will call array_unique and array_merge with thousands of values.

In one of my tests, Cache::mergeTags was called 71503 times and the average amount of tags in these calls was 2099.

I would like to hear if Drupal itself can benefit from optimizing merge, I'm not sure if this is only a GraphQL issue or maybe just my edge case.

Added patch for sure will fail tests and included just if someone would like to do quick profiling.
In my case, it reduces this query https://pastebin.com/ULmCsr3L from 12-13s to 4-5s. (Without Xdebug/profiler, the profiler doesn't show the same relative difference).
Also in the patch I've changed \Drupal\Core\Access\AccessResult::inheritCacheability, Cache::mergeMaxAges call was redundant since it is already done by addCacheableDependency. It seems to be a safe change and can be a different issue, although it doesn't change much in terms of performance.

Steps to reproduce

  • Create CacheableMetadata object
  • Keep adding tags to it
  • Profile

Proposed resolution

Change addCacheTags/Contexts so that merge is only performed when needed, for example only when calling getCacheTags/Contexts

Remaining tasks

User interface changes

None

API changes

TBD

Data model changes

-

Release notes snippet

-

📌 Task
Status

Active

Version

11.0 🔥

Component
Cache 

Last updated 2 days ago

Created by

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

Merge Requests

Comments & Activities

  • Issue created by @reinfate
  • last update about 1 year ago
    Custom Commands Failed
  • last update about 1 year ago
    Build Successful
  • Status changed to Needs review about 1 year ago
  • Status changed to Needs work about 1 year ago
  • 🇺🇸United States smustgrave

    Seems to have caused test failures.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 138s
    #342068
  • So another attempt for this.

    Here is simple reproduction

        $markups = array_fill(0, 20000, 'markups');
        $build = [];
        foreach ($markups as $i => $markup) {
          $build[] = [
            '#markup' => $markup,
            '#cache' => [
              'tags' => [(string) $i],
            ],
          ];
        }
        $render = \Drupal::service('renderer')->render($build);
        return new Response($render);
    

    Rendering this with #cache takes about 5-8 times longer on my machine than without them
    Mainly due to array_unique being called 20_000 times on the array that is growing from 1 tag to 20_000 tags

    Changing how public methods in Cache and Cacheable object work (to avoid calling array_unique on each cache tag merge) will inevitably fail tests and potentially break some user code.
    But that said, I've looked that each CacheBackend from core does another array_unique call before writing cache, so I would guess that even if we remove array_unique call in \Drupal\Core\Cache\Cache merge methods, everything still should work.

    But to be safe I've added additional mergeFast methods in the Cache-related classes and made use of them in the Renderer.
    Attached screenshot is the profiler isolated to \Drupal::service('renderer')->render($build); call from the example

    Lets see if this fail tests

  • Pipeline finished with Failed
    about 1 month ago
    Total: 660s
    #342078
  • Pipeline finished with Failed
    about 1 month ago
    Total: 507s
    #342119
  • So seems like only \Drupal\Tests\views\Kernel\Handler\FieldRenderedEntityTest::testRenderedEntityWithoutAndWithField failed.
    The problem with this test is that it checks that #cache tags and contexts are exact arrays from \Drupal\Tests\views\Kernel\Handler\FieldRenderedEntityTest::assertCacheabilityMetadata. But these render arrays do not have the #cache['keys'] so updated renderer do not remove duplicates from the cache metadata (as it assumes element will not be cached anyway)
    So either we can modify the test, or modify Renderer/RenderContext to apply cache to the element when RenderContext size is 1 (as it indicates that it is the top-most element for the current render() call)

    Moving to needs review, since I would like to get feedback on this whole issue before proceeding further.

Production build 0.71.5 2024