Outbound path & route processors must specify cacheability metadata

Rerolled against HEAD, but will surely fail tests. I just want a review of the approach, so marking as "do not test".

In this reroll, I'm no longer changing MenuLinkBase::isCacheable() — it made little sense after all. Now only changing MenuLinkContent. That makes the patch much, much simpler.

Also added test coverage.

I am not sure whether this condition is actually the proper way, honestly. Yes for HEAD this might be true, though, the same cacheability issue could easily happen for path processors as well.

Do you mean there might be other ways for links to be dynamic, and therefore non-cacheable? Please clarify.

Also: can we detect the information on save time of the menu item into the tree storage?

I thought the same initially, but I don't think that's worth it. By definition, isCacheable() must be called rarely: only on render cache misses. Hence doing this at save time and saving it into tree storage feels like a premature optimization. Let's make it correct first, in the simplest way possible, and then we can see if it negatively affects performance. (In HEAD, nothing calls isCacheable(), so it's certain this does not introduce a performance regression.)

Note: This applies patterns usually detects that on route compile time, can we do the same? Store the route processors on route options?

This too is an optimization. This issue is about correctness.
Nothing is new here, I only moved the "does this apply?" logic out of the existing method!

Better title.

I also really fear that all our caching optimizations will make it literally impossible to swap out any service, as we couple core to its own implementation details.

If that is happening somewhere, it's definitely not here. This issue is only making the implementation comply with the interface/API.

Well there are also path processors for example, which do change the path as well at the end of the day, and you can't really know how. (url alias by language for example).

You're right.

As you say, path processors can make arbitrary changes, and e.g. the language negotiation by URL path processor alters every single path. So, theoretically speaking, whenever any path processor is enabled, MenuLinkInterface::isCacheable() would always have to return FALSE.
But… practically speaking:

• we render cache everything per language
• the language negotiation by URL path processor is the only path processor (in core)
• hence as long as menus are render cached per language, having ::isCacheable() return TRUE when the language negotiation processor running should actually make it return FALSE will not cause any bugs
• a ::isCacheable() implementation that lies less is better

So, while this patch is definitely imperfect, it also definitely moves us in the right direction. Just like the patch that added MenuLinkInterface::isCacheable() clearly was also imperfect but landed anyway (otherwise this issue wouldn't exist) because it moved us in the right direction.

I don't really get why this patch is 100% critical

This blocks #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, which is critical.

Well, it does not happen in core, but do you know that contrib does?

I agree, and I said that in #12 also. And in fact, this partially fixes an existing security problem in HEAD: the current implementation of ::isCacheable() (which always returns TRUE) causes code to assume that links are cacheable that aren't. This can easily lead to security problems, such as session tokens being stolen.

Let me repeat:

So, while this patch is definitely imperfect, it also definitely moves us in the right direction. Just like the patch that added MenuLinkInterface::isCacheable() clearly was also imperfect but landed anyway (otherwise this issue wouldn't exist) because it moved us in the right direction.

Let's slowly get this back on track, step 1: fixing the title.

1. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
   @@ -47,6 +48,8 @@ public function processOutbound($route_name, Route $route, array &$parameters) {
   +      // Tokens are per user and per session, no not cacheable.
   

   s/no not/so not/

2. +++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
   @@ -20,17 +20,17 @@
   -  public function processOutbound($path, &$options = array(), Request $request = NULL);
   +  public function processOutbound($path, CacheableMetadata $cacheable_metadata, &$options = array(), Request $request = NULL);
   
   +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -288,7 +291,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
   +    $path = $this->processPath($path, $options, $cacheable_url_value);
   

   Parameter order mismatch is causing the test failures.

3. +++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
   @@ -7,11 +7,14 @@
   + * @todo - remove this class when we remove UrlGenerator::fromPath().
   

   Is there an issue for this already that this can link to?

4. +++ b/core/lib/Drupal/Core/Routing/CacheableUrl.php
   @@ -0,0 +1,39 @@
   +   * The string value of the url.
   

   s/url/URL/

5. +++ b/core/lib/Drupal/Core/Routing/CacheableUrl.php
   @@ -0,0 +1,39 @@
   \ No newline at end of file
   

   .

Fatal error: Cannot use object of type Drupal\Core\Routing\CacheableUrl as array

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -486,9 +487,11 @@ protected function processPath($path, &$options = array(), CacheableMetadata $ca
+   * @param CacheableMetadata $cacheable_metadata

Needs FQCN.

Green patch! :)

dreditor review:

1. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
   @@ -37,7 +36,7 @@ function __construct(CsrfTokenGenerator $csrf_token) {
   +  public function processOutbound($route_name, Route $route, array &$parameters, CacheableMetadata $cacheable_metadata = NuLL) {
   

   s/NuLL/NULL/

2. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -129,4 +129,74 @@ public function setCacheMaxAge($max_age) {
   +   *   The other bubbleable metadata object.
   

   s/bubbleable/cacheable/

3. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -226,12 +226,17 @@ public function build(array $tree, $level = 0) {
          $element = array();
          $element['attributes'] = new Attribute();
          $element['attributes']['class'] = $class;
   -      $element['title'] = $link->getTitle();
   -      $element['url'] = $link->getUrlObject();
   -      $element['url']->setOption('set_active_class', TRUE);
   +      $element['link']['#type'] = 'link';
   +      $element['link']['#title'] = $link->getTitle();
   +      $element['link']['#url'] = $link->getUrlObject();
   +      $element['link']['#url']->setOption('set_active_class', TRUE);
   +      $element['link']['#cache'] = array();
   +      if (!$link->isCacheable()) {
   +        $element['#cache']['max-age'] = 0;
   +      }
          $element['below'] = $data->subtree ? $this->build($data->subtree, $level + 1) : array();
          if (isset($data->options)) {
   -        $element['url']->setOptions(NestedArray::mergeDeep($element['url']->getOptions(), $data->options));
   +        $element['link']['#url']->setOptions(NestedArray::mergeDeep($element['link']['#url']->getOptions(), $data->options));
          }
          $element['original_link'] = $link;
   

   First thought: why keep ['attributes'] and not just move that to ['link']['#attributes']?

   Second thought: why even have ['link'] be at a nested level, why not set all of this directly on $element?

   Third thought: oh, we have ['original_link'] and especially ['below'], that's probably why. We definitely can't get rid of the latter.

   That then just leaves the first thought/question.

   EDIT: the menu.html.twig template gave me the answer: because the attributes apply to the menu link item, not the link itself. I.e. the template applies these attributes to the <li>, not the <a>.

4. +++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
   @@ -20,17 +20,17 @@
   +   *   Optional object to collect information about cache contexts and tags.
   
   +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -483,9 +487,11 @@ protected function processPath($path, &$options = array()) {
   +   *   An object to collect information about cache contexts and tags.
   
   +++ b/core/lib/Drupal/Core/Url.php
   @@ -728,15 +729,18 @@ public function setAbsolute($absolute = TRUE) {
   +   *  Optional object to collect cache tags and contexts for the URL.
   

   Also max-age. Why not just say (optional) Object to collect path processors' cacheability metadata.

5. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -9,6 +9,8 @@
   +use Drupal\Core\Url as CoreUrl;
   

   Why is this aliasing necessary?

6. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -75,9 +77,14 @@ public static function preRenderLink($element) {
   +      $merged = $cacheable_metadata->merge(CacheableMetadata::createFromRenderArray($element));
   +      $merged->applyTo($element);
   

   Alternatively: RendererInterface::addDependency().

   So these two lines would then become:

   $renderer->addDependency($element, $cacheable_metadata);
   

Next: applying patch + deeper review.

For the deeper review, the main thing I'm interested in, is confirming that rendered menus are now indeed varied by cache contexts as necessary. This is the main problem of #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

This is easy to check: manually added a route:<current> menu link item to the "Tools" menu, which renders a link to the current route (route name + route parameters). This means that the menu must now be varied by route name + parameters, of which there can be zillions of combinations. Confirm it is working by navigating your Drupal 8 site, and confirm that the menu link you created indeed always points to the current URL!

The remaining problem to solve is to avoid creating three zillion variations: to automatically turn the parts that vary by a "high-frequency cache context" (such as the route and user cache contexts) into #post_render_cache placeholders, so that we can render only those dynamically. We can deal with that in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
If you tried the example scenario above, you can look at the cache_render DB table, looking at the entries that contain the string "tools", and confirm that there are many different entries, one per route you visited.

We'll want test coverage that verifies this is working correctly. I can do that. Basically: an integration test for the changes made in this patch, to confirm that the cacheability metadata for the generated links bubbles up to the render cached menu block.

Finish the changes to CacheableMetadata, including test coverage. It'd be better if we'd move all of those changes into a separate issue; they're not actually in scope for this issue, but they are necessary.

Voila, now split #61's changes that affected CacheableMetadata + BubbleableMetadata to #2474121: CacheableMetadata should get BubbleableMetadata's merge/applyTo/createFromRenderArray/createFromObject methods.

#58:

• RE: CoreUrl: d'oh, right!
• RE: keeping the merge code: I'm fine with that. But I don't think it's more understandable.

#60:

• This should not be necessary. Because this is twig the link() function can just be changed to return a render array.

  Ah, interesting, we didn't think of that :)

• The DX I would have liked is something to: […]

  That is interesting. We currently have this complex mess interaction between 4 things, with 2 symmetries: Url + Link value objects and UrlGenerator(Interface) + LinkGenerator(Interface) services. If we'd only have Url, and only UrlGenerator, and LinkGenerator would be deleted in favor of a Url::generateLink($text) or Url::toLink($text)method on the Url value object (much like it already has toString()), and a matching UrlGenerator::generateLink(string $text, Url $url), then things would be simpler: there'd be less confusion about which value objects/services to use best: there'd only be one choice.
  But… I think such changes are too late by now.

• Could we at least for the link generator add the cacheable_metadata to the CoreUrl instead?

  Or is it a no go that we change the url itself?

  This is what pwolanin did at first, but it requires changes all over core, and with >90% certainty, also lots of changes to contrib. There are also still valid uses of generating URLs and links where cacheability doesn't matter (e.g. e-mails — I wanted to say feeds and REST responses too, but that's not really true).
  So, yes, this was done to minimize BC break.

• Not yet discussed with anyone, but maybe having a Link object might be beneficial, but on the other hand there is still Url->toString() ...

  We already have that: \Drupal\Core\Link. The problem, as I said above, is that it's not clear which to best use when.

• I am not yet convinced that the approach of passing the cacheable_metadata everywhere is the right one. It is a huge step forward, but especially the ease of use of Url->toString() and having no meta data available inside the url even though an url was generated makes me dizzy ...

  Until #39, pwolanin was letting UrlGenerator return a CacheableUrl object, which had a __toString implementation. We though that'd be sufficient for BC. Turns out it wasn't.
  So starting in #40, he embarked on the current route (pun intended).

• Why not […] $url->toRenderArray() […] instead?

  We considered that also. I'm not sure anymore why we dismissed that. IIRC there was some problem with that. Could you chime in WRT that, @pwolanin?

+++ b/core/lib/Drupal/Core/Render/Element/Link.php
@@ -75,9 +77,14 @@ public static function preRenderLink($element) {
+    if (!empty($element['#url']) && $element['#url'] instanceof CoreUrl) {

Why is this instanceof check actually even necessary? It wasn't there before…

#2474121: CacheableMetadata should get BubbleableMetadata's merge/applyTo/createFromRenderArray/createFromObject methods landed, rebased this patch on top.

The rebase-interdiff.txt shows a small change that should've been made as part of #2474121, but was forgotten. But, instead of representing >10 KB of the changes in this patch, it's now only a few lines of changes, thanks to #2474121 having already landed.

The test coverage added in #2474055: Performance regression in contact_help() now needs to be updated too, which explains the first test failure. The second failure is AFAICT unrelated — it even occurs on my system with HEAD (probably because my computer has apc.enable_cli = 0).

Fixing what's in #69's rebase-interdiff in #2475397: Tiny follow-up for #2474121 + unit test coverage for (Bubbleable|Cacheable)Metadata::merge().

#73: #74 is exactly why we did not add a session cache context in that issue.

#2475397: Tiny follow-up for #2474121 + unit test coverage for (Bubbleable|Cacheable)Metadata::merge() landed. Rebased.

Next: tests.

And voila, test coverage.

There's no real good place to test this, unless we create mocked route & path processors. So instead, I opted for an integration test that tests existing outbound route/path processors where available, to test the entire gamut of possibilities.

If not deemed elegant enough, then it should at least be a good starting point, that shows what needs to be tested.

I'll update the IS later today.

#81: yeah, the only major change is the KTB test. I agree that it's far from ideal. But cleaning up all the test coverage for path & route processors would be out of scope too.

#82+ #83: it's not clear to me either what needs to happen. Also note that I replied to everything you wrote in #60 point-by-point in #63.

Could you please clarify your concerns?

Addressed the immediate concerns in #82/#83/#87: menu.html.twig no longer using link(). There is an obvious solution, that I double-checked with Fabianx to be acceptable: let link() return a #type => link render array! Then we just get the same bubbling of cacheability metadata like we have for any render array. As a consequence, we can then remove a bunch of the changes we were making to templates, and simultaneously really make sure that outbound path & route processors' cacheability affect all links, not just those in menus.

The one thing that is incompatible with that is:

     if (!$link->isCacheable()) {
       $element['#cache']['max-age'] = 0;
     }

But, really, that is a separate problem. Ironically, it was the original goal of this issue ("fix MenuLinkInterface::isCacheable()"), but clearly the focus of this issue has shifted to the outbound route & path processing. That applies to all links & URLs. The MenuLinkInterface::isCacheable() part, and therefore the bit of cited code above, that really belongs in a separate issue.
So I opened #2479767: Fix MenuLinkInterface::isCacheable(): remove it in favor of implementing CacheableDependencyInterface for that.

Also, first step in cleaning up the IS.

This reroll makes any Twig template using link() bubble cacheability metadata. But what's still missing, is url(). Doing that next.

After that, I'll address #88. (Which will include making the IS 200% better.)

This reroll makes any Twig template using link() bubble cacheability metadata. But what's still missing, is url(). Doing that next.

That's what this does.

Next: #88, and absolutely agreed that we need more/better test coverage, especially with the interdiffs in #90 and here!

First, fixing test failures.

A first round of making the IS actually say what the patch does. Still working towards answering #88…

Fixing many minor things that I encountered while writing this IS update.

While doing #94, I think I also noticed these two out-of-scope changes. Let's see if I was right in thinking these were out of scope: if they were, they shouldn't cause any test failures.

Failures because:

• in #94, I fixed UrlGeneratorInterface to have the necessary updates, but I forgot to update a test, thus causing a fatal.
• in #95, the first hunk is a necessary bugfix, restoring that.

Such n00b.

I re-read the entire issue, to correctly answer #88.

#23:

See also #2417793-46: Allow entity: URIs to be entered in link fields, where changes to URL aliases don't result in a cache clear of rendered entities containing formatted links. Maybe that needs its own issue, but seems highly related to this, so just noting it here for now.

This is exactly what I feared was the case. Note that when changing the URL alias of a node, the node updates a MenuLinkContent entity. Which means everything is updated correctly. It is only when updating URL aliases via /admin/config/search/path, that no invalidation happens. But, to be honest, that's really a problem independent of this issue: URL aliases don't track changes and hence also not invalidations in any way. This issue is about fixing the more general problem, that sits a level higher: to make it possible for those outbound path/route processors that do have cacheability metadata, to be able to associate that cacheability with generated URLs.
So we should fix URL aliases in a separate issue. #2480077: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags

#88:

I'm confused, given that the patch doesn't add test coverage for the various changes in the route processors out there. Don't we want to check whether the cache contexts are adding for those ones? Isn't that the entire point of the issue, so why should we not expand the test coverage.

The problem is that none of those outbound path/route processors, with the sole exception of the CSRF token route processor, already has test coverage. Creating test coverage for all these seems out of scope here. Providing integration test coverage to show that this works from the lowest level (i.e. the path/route processors adding cacheability metadata) up to the highest level (i.e. the aforementioned cacheability metadata is present on rendered menus that contain links using path/route processors) seems sufficient to prove that this issue is working correctly?

Anyone mind to have a look at #84? I still don't get why we can't use a similar approach to the access result object one.

To properly answer this, I re-read the entire issue. Hence also my reply to #23 above, which wasn't addressed yet either. Apparently you've been saying this since #38, then again in #40, then in #84. Every single time, I didn't understand what you meant; I thought you meant that we shouldn't pass CacheableMetadata around, but instead return a value object. I was going to answer that that is what @pwolanin had tried until #40, but he changed direction because it'd have required too many changes.
But what you actually were trying to say was:

Why not use something like this?

   * @return bool|\Drupal\Core\Access\AccessResultInterface
   *   The access result. Returns a boolean if $return_as_object is FALSE (this
   *   is the default) and otherwise an AccessResultInterface object.
   *   When a boolean is returned, the result of AccessInterface::isAllowed() is
   *   returned, i.e. TRUE means access is explicitly allowed, FALSE means
   *   access is either explicitly forbidden or "no opinion".
   */
  public function access(AccountInterface $account, $return_as_object = FALSE);

And that is an excellent, excellent question! Trying that now :)

Sorry for all the confusion & frustration :(

Having done #101, I was forced to look at all changes made by this patch. And noticed that several spots were missed in the prior patches.

And that … got me in the fun, sweet "D8 URL generation, WTF-is-this'-reactions" land.

IMHO the real cause of that is the absolutely horrendous(ly fragmented) system we have to generate URLs and links in Drupal 8. It doesn't make sense to have both UrlGenerator and Url::toString(), to have both #type => link and LinkGenerator. In all of these cases, there are far too many options. Url::toString() is about generating a string, and for strings, we shouldn't care about cacheability; we only care about cacheability of render arrays.
If this patch is frustrating, then >=80% of that frustration originates from that area. It's probably completely due to legacy reasons that were all reasonable along the way, but we're definitely going to be bitten by broken edge cases there during the D8 cycle.
</rant>

Anyway, this is what that approach would look like. Do you think this is better, @dawehner? I honestly don't care, because as per the above rant, it's already horrible anyway.

Forgot to update one small thing; AFAICT that's causing all these test failures.

Those last 7 failures I also discovered locally, while working on test coverage. Fixed it.

Also, while working on the test coverage, I noticed #2480233: Incorect docblocks in route processor tests and @xjm kindly fixed it.

This patch adds complete test coverage, including for path processors that didn't even have any explicit test coverage previously. As you can see in the interdiff, the test coverage adds another ~40 KB of patch weight: 12 files changed, 391 insertions(+), 62 deletions(-).

This took me ~12 hours to get sorted out! I could write a complementary rant (see #101) about URL generation test coverage, but I won't.

CR created: https://www.drupal.org/node/2480761.

Fixed \Drupal::url() (99% of that is in tests and in hook_help()), and \Drupal::l(). CR updated.

Hah, this uncovered a small bug in \Drupal\views\Plugin\views\field\FieldPluginBase, which would've prevented Views' "more" links from having the views-more-link class :) That's what caused the two failures.

1. catch asked this in #73, see #74 + #77 for the answer. In other words: yes, that'd be ideal, and Fabianx has been proposing to add a "CSRF token" cache context. But in any case: that can easily be done in a follow-up. In the reality of HEAD, max-age = 0 is appropriate.
2. Good point. Done.
3. Heh :)
4. No reason; that's my preferred code style, and therefore it's still in my fingers. Changed that to your liking — I don't care.
5. Hah, I was thinking about that too! This only captures \Symfony\Component\HttpFoundation\Request::getSchemeAndHttpHost(), so, no, base path is not included. Fixed that. Renamed the cache context to SiteCacheContext, and the cache context ID to url.site.
6. Well, this code is definitely very tricky. For authenticated users, no outbound processing is ever done. For the anonymous user, we only do outbound processing if the current request actually has the configured query parameter. So, in other words: this was plain wrong. It is nonsensical to say that the output varied by the URL language, because this enabled other code to depend on the URL language. But this code itself actually depended on the configured query parameter, for anonymous users. i.e. it ensures that if anonymous users can specify "mysite.com/blog?lang=de", that all generated links on that page would also continue to have that "?lang=de" query parameter.
7. That was just copy/pasted from the other test method on that class, but sure, just updated both to use that. Much nicer indeed :)
8. Done: #2481833: Remove LanguageNegotiationUrl's usage of base_path().

1. Good catch, fixed.
2. HAH! See the #101 interdiff — until that patch, that's exactly what this was doing. @dawehner felt it'd be much better to go this way. See #100. This approach is consistent with how AccessibleInterface::access() works (the $return_as_object = FALSE thing).
   I don't have a preference. So it's up to you & dawehner.

#119: But that's a bigger API change.

Sorry, forgot to upload the patch accompanying #117. Here it is.

#121: Thanks for the review!

1. Hm… looks like you're right. That makes sense, actually. So then I need to add a new cache context. What about url.site.basepath?
2. No, url.site = host + port + base path, i.e. the value for the url.site cache context is calculated without path processing.

#130: I'm fine with string|CacheableUrl. That's basically what we had in #40. But back then, we were always returning CacheableUrl, and that is what was infeasible. (EDIT: And I agree that that is better, and more similar to AccessibleInterface::access([…], $return_as_object = FALSE).)

Would we all be fine with that?

14:05:10 dawehner: WimLeers: if you implement string|array now and you should be pretty much possible to implement an object with BC layer later
14:05:33 WimLeers: dawehner: but why not do it the other way around then? Won't that be more elegant in the future?
14:05:36 WimLeers: Fabianx-screen: ^^
14:07:05 dawehner: WimLeers: well, the other way round IMHO doesn't make sense as we already have the object
14:07:18 dawehner: i was proposing that in order to move forward on that issue
14:07:30 WimLeers: hehe I know :)
14:07:39 WimLeers: And I appreciate that!
14:08:00 dawehner: WimLeers: yeah no idea about #31 to be honest
14:08:03 WimLeers: But this will be seen by almost every Drupal developer, so if an object is better, then I'm totally fine with rerolling it
14:08:22 dawehner: WimLeers: i thought that return as string will be still the default?
14:08:46 WimLeers: yes, it will be
14:09:04 WimLeers: dawehner: but returning an object would be consistent with how AccessResults are done
14:09:31 WimLeers: dawehner: and it'd shows the right pattern for contrib developers: value objects
14:09:34 WimLeers: not multiple return values
14:09:52 WimLeers: All across core we use value objects in favor of multiple return values AFAIK?
14:11:11 dawehner: yeah right
14:11:14 dawehner: so let's go with an object

So we'll go with #130/#131.

#133:

(Refreshed until the number of function calls stabilized.)

/admin/index

HEAD

241,178 function calls (yes, 241K!)

Patch

242,586 function calls

Conclusion

1408 additional function calls, or 0.58% more.

/node/1

HEAD

90,146 function calls (90K)

Patch

91,540 function calls

Conclusion

1394 additional function calls, or 1.5% more.

However, this is being done to allow for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, which provides much bigger gains than what we regress here.

Let's see how well this fares. Did #130/#134.

All unit tests pass, clicking around the UI works. Let's see if I missed anything.

I did another round of profiling to answer #138, and to my surprise… the numbers are vastly different now. And reproducibly so.

HEAD (e0aae8c26ae3367b96d2e49a8beb05a441a9f8fc) still has the same number of function calls as in #135, thus indicating I didn't test incorrectly.

But the number of function calls with patch #137 is not 90,164 at /node/1 (as was tested in #135 with patch #117), but … 87,435. Or a reduction of 2729 function calls, which means 3.0% fewer function calls with this patch!

I don't get it either. But I can consistently reproduce this with the patch in #137. The main reasons: 226 fewer function calls (than HEAD!) for Cache::mergeContexts(), 213 fewer for Cache::mergeTags(), which then adds up to ~3K less function calls because of the logic in those two methods. Which makes it unsurprising that #2408013: Adding Assertions to Drupal - Test Tools. brings such a nice performance boost.

It'd be great if somebody could repeat the profiling to confirm my numbers.

+++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
@@ -140,8 +140,8 @@ public function setCacheMaxAge($max_age) {
-    $result->contexts = Cache::mergeContexts($this->contexts, $other->contexts);
-    $result->tags = Cache::mergeTags($this->tags, $other->tags);
+    $result->contexts = empty($this->contexts) ? $other->contexts : Cache::mergeContexts($this->contexts, $other->contexts);
+    $result->tags = empty($this->tags) ? $other->tags : Cache::mergeTags($this->tags, $other->tags);

This is why I thought I was going insane.

Many thanks to @dawehner for pointing that out and helping me recover my sanity.

Opening #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() to do that first, since it presents such a significant performance boost.

YAY, green!

So, ignore #139.

Profiling /node/1 again, with #137:

HEAD

90,164 function calls

HEAD + #2483433-1: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()

(which now is responsible for landing the code cited in #140, which was the reason why #139 showed those confusing numbers)

86,298 function calls

HEAD + #2483433-1: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() + #137

87,435 function calls

That's 1137 additional function calls. The difference indeed lies in the collecting and merging of cacheability metadata.

But:

1. that can be optimized, which is what we're doing in #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() — the latest patch there shaves another few thousand function calls off of /node/1, and would cause that 1137 additional function calls to drop to <1000 additional function calls
2. we always knew there would be *some* cost to this; #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees needs it, and is able to save many more function calls
3. finally: with #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees applied, the delta in number of function calls for "regular links" should be zero, because we only do the extra work for links that indicate they want to collect cacheability metadata

#143: glad you saw that :) I considered that too, but figured it'd be more important to keep the number of changes as low as possible in a first iteration :) Let's do it now!

Cross-posted :) I think mine is more clear (because I remove $cacheable_metadata, rather than renaming $generated_url to $cacheable_metadata), and you forgot to update UnroutedUrlAssembler. However, you also did find one more possible optimization than I did: the one in LinkGenerator. Fixing that too now.

Did #147.

Also, thanks to @larowlan/#145/#146/#148, this no longer uses any CacheableMetadata::merge() calls, thus allowing me to revert the changes cited in #140. That's now entirely in the domain of #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge(), meaning that this is no longer blocked on that issue!

Updated IS for #134/#137–#148. Also updated CR.

Well, I think we should have a close look at what costs these additional function calls, ... we can not just look at entirely warm requests.

Another round of profiling to answer this.

/node/1/

HEAD

90,164 function calls

Patch #148.

91,720 function calls

+1556 function calls

/node/1/ + #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()

HEAD + #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()

82,961 function calls

Patch #148 + #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()

83,538 function calls

+557 function calls

And yes, when looking at it with XHProf, we can clearly see it's completely due to the cacheability metadata (this is with HEAD, without #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()):

But, with #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() applied, that picture becomes quite different, as the much lower increase in number of function calls suggested: