Metatag discards cacheability metadata, results in a performance hit

1. +++ b/metatag.module
   @@ -234,25 +234,25 @@ function _metatag_remove_duplicate_entity_tags(array &$build) {
   +      $duplicates = array_intersect_key($name_map , $duplicates);
   +      foreach ($build['#attached']['html_head_link'] as $key => $item) {
   +        if (!$duplicates) {
   +          // We've found all items.
   +          return;
   

   why is the !duplicates check inside the foreach?

2. +++ b/metatag.module
   @@ -415,18 +415,27 @@ function metatag_preprocess_html(&$variables) {
   + * @param null $entity
   + *   Entity being viewed if appropriate.
   

   that doesn't seem right :) (null)

3. +++ b/metatag.module
   @@ -415,18 +415,27 @@ function metatag_preprocess_html(&$variables) {
   -function metatag_get_tags_from_route($entity = NULL) {
   +function _metatag_get_metatag_info_from_route($entity = NULL, array &$context = []) {
   +  $metatags = &drupal_static(__FUNCTION__);
   +  if (!is_null($metatags)) {
   +    return $metatags;
   +  }
   

   this gets $entity and $context, but the static cache doesn't consider those arguments.

   Also, isn't renaming this function a BC break?

4. +++ b/metatag.module
   @@ -452,7 +461,27 @@ function metatag_get_tags_from_route($entity = NULL) {
   +/**
   + * Load the meta tags by processing the route parameters.
   + *
   + * @return mixed
   + *   Array of meta tags or NULL.
   + */
   +function metatag_get_tags_from_route($entity = NULL) {
   +  $metatag_manager = \Drupal::service('metatag.manager');
   

   Ah, the old function is readed down here, that's a bit confusing, can we move this up to have an easier to read patch?

5. +++ b/metatag.module
   @@ -452,7 +461,27 @@ function metatag_get_tags_from_route($entity = NULL) {
   +  $context = [
   +    'entity' => &$entity,
   +  ];
   +  $metatags = _metatag_get_metatag_info_from_route($entity, $context);
   

   this seems like a pretty strange API, entity is passed in twice?

With primed render cache, this no longer shows up at all.

🥳

So I think perhaps something in token generation was resulting in the render caching being bypassed for the entity view.

That's a very good hunch!!! See #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case. That added

   * @param \Drupal\Core\Render\BubbleableMetadata|null $bubbleable_metadata
   *   (optional) An object to which static::generate() and the hooks and
   *   functions that it invokes will add their required bubbleable metadata.
   *
   *   To ensure that the metadata associated with the token replacements gets
   *   attached to the same render array that contains the token-replaced text,
   *   callers of this method are encouraged to pass in a BubbleableMetadata
   *   object and apply it to the corresponding render array. For example:
   *   @code
   *     $bubbleable_metadata = new BubbleableMetadata();
   *     $build['#markup'] = $token_service->replace('Tokens: [node:nid] [current-user:uid]', ['node' => $node], [], $bubbleable_metadata);
   *     $bubbleable_metadata->applyTo($build);
   *   @endcode
   *
   *   When the caller does not pass in a BubbleableMetadata object, this
   *   method creates a local one, and applies the collected metadata to the
   *   Renderer's currently active render context.
…
    // If a local $bubbleable_metadata object was created, apply the metadata
    // it collected to the renderer's currently active render context.
    if (!$bubbleable_metadata_is_passed_in && $this->renderer->hasRenderContext()) {
      $build = [];
      $bubbleable_metadata->applyTo($build);
      $this->renderer->render($build);
    }

to \Drupal\Core\Utility\Token::replace(), and as you can see, it automatically bubbles the cacheability to the global render context if the optional parameter is not provided.

And … \Drupal\metatag\MetatagManager::generateRawElements() does not provide it:

        $processed_value = PlainTextOutput::renderFromHtml(htmlspecialchars_decode($this->tokenService->replace($tag->value(), $token_replacements, ['langcode' => $langcode])));

Except that $this->tokenService is not \Drupal\Core\Utility\Token, but \Drupal\metatag\MetatagToken, but that's effectively just a decorator:

  public function replace($string, array $data = [], array $options = [], BubbleableMetadata $bubbleable_metadata = NULL) {
…
    $replaced = $this->token->replace($string, $data, $options, $bubbleable_metadata);

So, yes, I think your hunch was onto something 👏: \Drupal\metatag\MetatagManager::generateRawElements() should be passing in that optional parameter.

I don't have time to dig in more right now, but it's of course interesting that you solved your problem without addressing that. Perhaps that's because there's multiple invocations of that method and your static cache prevents the invocation that is happening in head while executing \metatag_page_attachments(), hence indirectly also avoiding the bubbling onto the current render context?

This needs additional work, right? Reading #11 and #12 it seems like the test coverage might need to be expanded slightly to ensure that meta tags output correctly on the second page request, not just the first request, right?

Any thoughts on the test failures?

#20: The test expectations are not yet cache-aware, and hence need updating.

I pushed up a fix for the restui thing, I must have committed the wrong patch or something as the one I uploaded to the issue (#3123578) didn't have that line. Sorry about that.

+++ b/src/MetatagManager.php
@@ -493,11 +496,16 @@ class MetatagManager implements MetatagManagerInterface {
+    if (!$cache) {
+      @trigger_error('Calling ::generateRawElements without cacheability metadata is deprecated in 8.x-1.13 and is removed from 8.x-2.x. @see https://www.drupal.org/node/3123628');
+    }

I find deprecation messages for contrib hard as there are usualy no clear rules when and if at all there will be a 2.x branch :) Probably should be 2.0.0 now? And following the new naming, should this be metatag:8.x-1.13 and metatag:2.0.0 respectively?

I don't quite understand how this affects caching. I'll try to do some profiling on my own over the next days.

@larowlan: Something just doesn't quite add up to me here :)

For me, despite having 45 calls to this function, it's only 3.6% (24ms) of the total page request with the current stable version of metatag. I see the red warning icon in your screenshot about xdebug being enabled, that *massively* slows down everything, the whole page was 2x as fast for me after disabling xdebug (this function was 38ms). Note that many small function calls also tend to be over-reported with blackfire, due to the overhead that each function call has.

What scenario are you measuring exactly, authenticated or not, render caches enabled?

Are you using any special tokens or so with special cacheability? Are you using property tokens ([node:field:value] instead of [node:field])? That can massively speed up token generation because the non-property tokens actually render the field into HTML through a formatter and then remove that again.

What I can reproduce on my site here is that the entity view alter hook *is* being called on what should be a render-cached page, so there is indeed something interesting going on.

But, I don't quite see why collecting the metadata like that would change whether or not elements are cached. We are currently in a pre render callback, so it should IMHO more or less end up in the same place. Unless we collect it but then lose the metadata as a result of this change, which would explain it being faster, but would also defeat the purpose of doing it :) Looking at it a bit closer, I'm pretty sure that's exactly what's happening. In _metatag_remove_duplicate_entity_tags(), it then has $metatag_attachments['#cache'] but the function has no return value, it only uses that to unset stuff in $build. If you'd actually merge things over into $build then I'm quite certain you'd end up right where you started.

I suspect that the reason you and I don't see render caching happening for the node is a different one. For you, it actually might be a token, and if that's the case, it would be the correct and expected behavior. For me, it's something else (possibly around access/translation stuff, aware of some issues there on this project), so this patch doesn't actually improve anything :)

What cacheability metadata do you see with this patch in your $cache? You might want to set a breakpoint on the foreach that does the token replace and detect which token causes that and make it use a different token :)

so, I'm reasonably certain this is a won't fix and your problem is somewhere else, in a specific token.

Thank you for the really detailed review, Berdir.

Putting this back to Needs Work as it seems this needs some further analysis.

On cool cache the time spent in metatag_entity_view_alter was ~700ms for me. However, as suggested in #32

Are you using any special tokens or so with special cacheability? Are you using property tokens ([node:field:value] instead of [node:field])? That can massively speed up token generation because the non-property tokens actually render the field into HTML through a formatter and then remove that again.

That led to a 37% improvement. We also have a special token that I'll need to check as well. Will report back.

You could also try my patch in #2955407: Static caching for token processing; iconv_substr() is super slow

As suggested by @Berdir, I created a separate issue to focus on extending the generateRawElements(). This includes related parts from #23.

See #3175269: Pass optional bubbleable metadata parameter to generateRawElements to avoid issues with early rendering and JSON:API.

Re-rolled patch to work with latest release.

See #32, the patch isn't correct and shouldn't be used. No need to reroll and you shouldn't use it. See my suggestions and related issue if you're looking for a performance improvement around metatags.

Updated patch based on 8.x-1.x with correct bubbleable metadata handling.

this comment been deleted

this comment has been deleted

Updated patch based on 2.1.x with correct bubbleable metadata handling.