- 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 7:33am 20 October 2023 - Status changed to Needs work
about 1 year ago 2:40pm 20 October 2023 - Merge request !10225Issue #3395304: Merging cache metadata is slow with thousands of tags added → (Open) created by reinfate
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 tagsChanging 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 exampleLets see if this fail tests
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.