Optimize CacheableMetadata::merge() + BubbleableMetadata::merge()

So yeah I was wondering whether we could optimize for ages as well.

I love how diagrams replaced readable data. Let's better add a pie chart as well ;)

I'm guessing the performance benefits are due to not doing:

\Drupal::service('cache_contexts_manager')->validateTokens($cache_contexts);

However this is the only place we do this so are we now skipping this completely for some contexts?

I think we can shave off some more function calls - since both $this->whatever and $other->whatever can be empty.

I tested this vs. #9 on the front page/minimal profile/no page_cache/logged out, and saved an extra 294 (see xhprof diff - the run1 is with the above patch applied). More verbose though.

No interdiff since all the lines change.

I like #15, too.

Still need to answer when we would actually validate cache contexts then ...

Well, this now potentially moves the validation to the first place where you have two cache contexts at the same time ... given that I'm wondering whether it should be moved to cache set instead?

Fair at least #9 doesn't, if we check both the right and left side, the merging could happen later.

It'd be even more verbose, but we could validate individual contexts if either is set, but without merging unless both are set?

Slightly related issue: #2488296: Improve post render cache placeholders generation performance.

Needs work, we need to decide on what to do with #23 / #8 still ...

On #8 + #23:

I think we need two modes in the end:

- Production mode - where performance trumps
- Debug mode, where you can exactly see what is responsible for the fails

Probably assertions would take care of that.

I think for now I'd keep the DX parity here (i.e. validate on set) - and assume that we'll get assertions in before 8.0.0 to further optimize the validation.

If we don't get assertions in, we could always add a follow-up if we think the performance gain is more important than the DX. But then we might as well use assertions at that point.

Rerolled and made another change to add mergeAttachments to BubbleableMetadata. I spoke to catch and we couldn't see a good reason this was callout out, and needing to be overridable. As the renderer service uses BubbleableMetadata directly, so another implementation if it wanted another implementation would do that there instead anyway?

A quick benchmark looks a bit like this,don't take these figures as gospel please:

UPDATE: Just spoke to Fabian too, he said he spoke to Wim and they came to a similar conclusion too. That sounds like playground stuff, but that's how it happened :)

Nice! Makes also things easier to follow

We don't have any test coverage for that function? I would have expected that things fail

I didn't look at the tests yet, So I would expect stuff to fail. I was just testing this out.

+ a lot, felt weird to have BubbleableMetadata::merge() call out to \Drupal::service('renderer') to deal with #attached

Does the new BubbleableMetadata::mergeAttachments() still need to be public ? ::merge() is the real thing, right ?

I think we might want to keep the mergeAttachments method on the renderer and interface, seems a few things use this directly. It can just call out to the new static method I guess? I just copied the docs do BubbleableMetadata::mergeAttachments and RendererInterface::mergeAttachments have the same docblock atm.

EDIT: sorry, messed up the interdiff :)

Still didn't look at test coverage changes.

\Drupal\system\Tests\Common\MergeAttachmentsTest is the test coverage you are looking for.
Beside from that call, there is 1 single proper function call, I think its fine to change the API?

I think the API change here is fine. If we add a bc layer that nobody uses then it's instant dead code, which just makes things harder to maintain/follow with no benefit.

Let's see how this gets on then.

Removed mergeAttachments from the interface, replaced the 2 usages outside of the renderer, moved the tests to BubbleableMetadataTest, so DrupalUnitTest > UnitTest - nice.

EDIT: It was a lazy man's interdiff. So the delete of MergeAttachmentsTest.php is not in there, but is in the actual patch :)

Oh, placeholders.

I think postRenderCache just needs to go?

Not a bad improvement. This is /admin/content without views render caching for anonymous user (with page cache disabled)

### FINAL REPORT

=== 8.0.x..8.0.x compared (557186d0ce293..557186fee3ad5):

ct  : 62,911|62,911|0|0.0%
wt  : 126,854|127,702|848|0.7%
mu  : 22,666,216|22,666,200|-16|-0.0%
pmu : 22,794,936|22,794,968|32|0.0%

=== 8.0.x..2483433 compared (557186d0ce293..55718713bc09d):

ct  : 62,911|56,525|-6,386|-10.2%
wt  : 126,854|122,735|-4,119|-3.2%
mu  : 22,666,216|22,665,072|-1,144|-0.0%
pmu : 22,794,936|22,782,776|-12,160|-0.1%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2483433           |     56,525 |     60,990 |     56,570 |     56,525 |     56,525 |
| 8_0_x             |     62,911 |     67,376 |     62,956 |     62,911 |     62,911 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2483433           |    122,735 |    168,099 |    127,682 |    126,084 |    133,862 |
| 8_0_x             |    127,702 |    171,335 |    132,693 |    130,573 |    140,132 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2483433           | 22,665,072 | 23,353,192 | 22,692,385 | 22,665,072 | 22,915,584 |
| 8_0_x             | 22,666,176 | 23,404,488 | 22,698,745 | 22,666,200 | 23,166,856 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2483433           | 22,782,776 | 23,506,152 | 22,810,537 | 22,782,776 | 23,033,096 |
| 8_0_x             | 22,794,968 | 23,568,072 | 22,828,003 | 22,794,968 | 23,296,536 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

+++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
@@ -206,4 +206,394 @@ public function providerTestCreateFromRenderArray() {
+    );
+    $this->assertSame($expected['#attached'], BubbleableMetadata::mergeAttachments($a['#attached'], $b['#attached']), 'Attachments merged correctly.');
...
+    );
+    $this->assertSame($expected['#attached'], BubbleableMetadata::mergeAttachments($b['#attached'], $a['#attached']), 'Attachments merged correctly; opposite merging yields opposite order.');
+
...
+    );
+    $this->assertSame($expected['#attached'], BubbleableMetadata::mergeAttachments($a['#attached'], $b['#attached']), 'Attachments merged correctly; duplicates are retained.');
+
...
+    );
+    $this->assertSame($expected['#attached'], BubbleableMetadata::mergeAttachments($a['#attached'], $b['#attached']));
+

Sounds like a good usecase for a @dataProvider? And maybe dedicated test methods ...

mit data providers

Nice descriptive test methods. Thank you!

This issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed dc53be5 and pushed to 8.0.x. Thanks!