Optimize Cache::merge*(), by only accepting 2 instead of N arguments

Still working on it. Yesterday I had some problems with the xhprof installation on mamp and osx.
I already made the modifications. Roughly the code for one of the function looks this:

  public static function mergeContexts($cache_context1 = array(), $cache_context2 = array(), $cache_context3 = array()) {
    // $cache_context_arrays = func_get_args();
    // $cache_contexts = [];
    // foreach ($cache_context_arrays as $contexts) {
    //   $cache_contexts = array_merge($cache_contexts, $contexts);
    // }
    // $cache_contexts = array_unique($cache_contexts);
    $cache_contexts = array_unique(array_merge($cache_context1, $cache_context2, $cache_context3));
    \Drupal::service('cache_contexts')->validateTokens($cache_contexts);
    sort($cache_contexts);
    return $cache_contexts;
  }

I left 3 arguments because of the following call:
Tests/Entity/EntityCacheTagsTestBase.php @400, @410
Is this the right way to approach it? I was going to profile it.

I have added the patch, but haven't managed to make a profile test.
Please somebody, add some xhprof results.

I will do some profiling for this.

I think, we also need to update the commenting part.

I have updated the methods comments. Please review.

I did some tests. I run each function 10.000 times, and with or without the pach, and here are the results:

mergeContexts

Before patch

10000 runs, total run time 3.8855350017548, max run time 0.0035789012908936, min run time 0.00031113624572754.

After patch

10000 runs, total run time 3.4474279880524, max run time 0.0020530223846436, min run time 0.00027203559875488.

mergeTags

Before patch

10000 runs, total run time 3.330482006073, max run time 0.0016181468963623, min run time 0.00025105476379395.

After patch

10000 runs, total run time 2.8155419826508, max run time 0.0014889240264893, min run time 0.00021004676818848.

mergeMaxAges

Before patch

with Cache::PERMANENT as one of the parameters

10000 runs, total run time 2.1057970523834, max run time 0.001248836517334, min run time 0.00015687942504883.

without Cache::PERMANENT

10000 runs, total run time 1.4308922290802, max run time 0.0014550685882568, min run time 9.1075897216797E-5.

After patch

with Cache::PERMANENT as one of the parameters

Cache::PERMANENT is first_parameter:
10000 runs, total run time 0.69816279411316, max run time 0.001704216003418, min run time 3.0994415283203E-5.
Cache::PERMANENT is second_parameter:
10000 runs, total run time 0.78123903274536, max run time 0.0024521350860596, min run time 3.2901763916016E-5.

without Cache::PERMANENT

10000 runs, total run time 0.9782989025116, max run time 0.00097084045410156, min run time 5.1021575927734E-5.

reroll

The patch fails because there are some places that use more than 2 arguments for Cache::mergeTags method.
Here is one example in EntityCacheTagsTestBase.php @337

  // Generate the cache tags for the (non) referencing entities.
    $referencing_entity_cache_tags = Cache::mergeTags(
      $this->referencingEntity->getCacheTags(),
      \Drupal::entityManager()->getViewBuilder('entity_test')->getCacheTags(),
      // Includes the main entity's cache tags, since this entity references it.
      $this->entity->getCacheTags(),
      $this->getAdditionalCacheTagsForEntity($this->entity),
      $view_cache_tag,
      ['rendered']
    );

I suggest further analysis of the opportunity of this optimization for Cache::mergeTags.

Called Cache::merge*() multiple times where it used more than 2 arguments.

Patch attached.

@JeroenT you can also update the comments as here, if it is ok.

@rbmboogie,

I'm sorry, I took #12 as startpoint.

I created a new patch.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -76,29 +70,25 @@ public static function mergeTags() {
+   *    Max age value to find..

One small typo coming from my patch, notice there are 2 points :)

Fixed comment + some failing tests.

As far as I have checked, at least the following tests:

• Drupal\block_content\Tests\BlockContentCacheTagsTest
• Drupal\comment\Tests\CommentCacheTagsTest
• Drupal\node\Tests\NodeCacheTagsTest

fail because of the following verification in EntityCacheTagsTestBase in line @426:
$this->verifyPageCache($nonempty_entity_listing_url, 'HIT', $nonempty_entity_listing_cache_tags);
Currently, I don't have more time to investigate the issue, as I will be leaving back home from the Drupal Dev Days.
If the issue is still unsolved by Monday, I can check it again.
Thanks for all the help.

@rbmboogie,

Thanks! I think i fixed the last failing tests now.

At DrupalCon La, workingon reroll

Rerolled patch with 2 merge conflicts
Merge conflict in core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php line 55
Merge conflict in core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php line 100

Tried applying the patch by JeroenT again with PhpStorm, got no conflicts. Patch attached.

This should resolve part of the failures.

This should fix all remaining failures.

Fixed a couple points with this patch:

1. Brought the patch up to date with latest commits.
2. Optimized the to avoie multiple frequent calls to Cache::mergeTags. the goal was to eliminate multiple calls that did not need to occur until after the array had been built for the last time.

1. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -124,12 +124,7 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +    $tags_page_1 = Cache::mergeTags($base_tags, $entities[1]->getCacheTags(), $entities[2]->getCacheTags(), $entities[3]->getCacheTags(), $entities[4]->getCacheTags(), $entities[5]->getCacheTags());
   

   Cache::mergeTags should only have 2 arguments, so these should split up into multiple calls

2. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -124,12 +124,7 @@ protected function assertCacheTagsForFieldBasedView($do_assert_views_caches) {
   +    $tags_page_1 = Cache::mergeTags($base_tags, $entities[1]->getCacheTags(), $entities[2]->getCacheTags(), $entities[3]->getCacheTags(), $entities[4]->getCacheTags(), $entities[5]->getCacheTags());
   

   Same as 1.

3. @@ -272,15 +265,9 @@ protected function assertCacheTagsForEntityBasedView($do_assert_views_caches) {
   +    $new_entities_cache_tags = Cache::mergeTags($entities[1]->getCacheTags(), $entities[2]->getCacheTags(), $entities[3]->getCacheTags(), $entities[4]->getCacheTags(), $entities[5]->getCacheTags());
   

   Same as 1.

Updated to fix a couple stupid syntax errors which were preventing tests from running. Sorry about that.

I'm not sure what comments 42 and 43 are referring to, as that code has been fixed with my patch. Unless there are any issues, I will promote this to needs review after tests have hopefully passed.

Moving to needs review to trigger the testbot.

I noticed a couple of weird things in the patch file that weren't in the interdiff so I went ahead and fixed those.

1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -76,29 +70,25 @@ public static function mergeTags() {
   +    if ($a == Cache::PERMANENT){
   ...
   +    if ($b == Cache::PERMANENT){
   

   Might as well use ===

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -368,8 +368,10 @@ public function resetCache(array $entities = NULL) {
   +      $tags = Cache::mergeTags($tags);
   
   +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -48,6 +48,16 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +      $cache_tags = Cache::mergeTags($cache_tags);
   
   +++ b/core/modules/block_content/src/Tests/BlockContentCacheTagsTest.php
   @@ -91,12 +91,16 @@ public function testBlock() {
   +    $expected_block_cache_tags = Cache::mergeTags($expected_block_cache_tags);
   ...
   +    $expected_entity_cache_tags = Cache::mergeTags($expected_entity_cache_tags);
   
   +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
   @@ -352,18 +352,36 @@ public function testReferencedEntity() {
   +    $referencing_entity_cache_tags = Cache::mergeTags($referencing_entity_cache_tags);
   ...
   +    $non_referencing_entity_cache_tags = Cache::mergeTags($non_referencing_entity_cache_tags);
   

   I don't understand these final calls with one argument. They look like a mistake. Can we either remove them or document them?

3. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -48,6 +48,16 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +        $this->getCacheTags(), // Block view builder cache tag.
   +        $entity->getCacheTags() // Block entity cache tag.
   ...
   +        $plugin->getCacheTags() // Block plugin cache tags.
   

   We don't do inline comments like this, and the second element is missing a trailing comma

Fixed test failure in TrackerTest.

Regarding comments from #53

1. Agreed, fixed.
2. I don't understand those either. Removed the calls. Testbot should agree that these weren't needed.
3. Comma's are fine, we can only add 2 items so the trailing comma's would make incorrect. Removed the comments though. They didn't really add anything tbh.

Oh for #53.2, you're right about the commas, I misread it as part of an array, not in a function call, so you are correct.

Looks good to me. Just two remarks:

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -24,19 +24,16 @@ class Cache {
+  public static function mergeContexts($a = array(), $b = array()) {

@@ -53,19 +50,16 @@ public static function mergeContexts() {
+  public static function mergeTags($a = array(), $b = array()) {

Can we add type hinting to the parameters. In this way we make sure that the parameters are an array. Like: (array $a = array(), array $b = array())

+++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
@@ -372,24 +388,37 @@ public function testReferencedEntity() {
-
+    $nonempty_entity_listing_cache_tags = Cache::mergeTags($nonempty_entity_listing_cache_tags);

Can you explain why you add this change. I do not think it is necessary.

Made changes as suggested by daffie in #56.

This is the right interdiff...

Patch applies again now, changed the typehinting to use short array syntax (array $a = [], array $b = []). I had to make a change in the TrackerTest as well to also check for the 'user.node_grants:view' cache context.

Fixed test failures and remarks from Wim in #61. I'm not really happy with the changes in EntityCacheTagsTestBase for $page_cache_tags. It does make sense to fix the mergeTags call but the merged comment doesn't feel right.

     // Cache tags present on every rendered page.
-    $page_cache_tags = Cache::mergeTags(
-      ['rendered'],
-      // 'user.permissions' is a required cache context, and responses that vary
-      // by this cache context when requested by anonymous users automatically
-      // also get this cache tag, to ensure correct invalidation.
-      ['config:user.role.anonymous'],
-      // If the block module is used, the Block page display variant is used,
-      // which adds the block config entity type's list cache tags.
-      \Drupal::moduleHandler()->moduleExists('block') ? ['config:block_list']: []
-    );
+    // 'user.permissions' is a required cache context, and responses that vary
+    // by this cache context when requested by anonymous users automatically
+    // also get this cache tag, to ensure correct invalidation.
+    // If the block module is used, the Block page display variant is used,
+    // which adds the block config entity type's list cache tags.
+    $page_cache_tags = Cache::mergeTags(['rendered', 'config:user.role.anonymous'], \Drupal::moduleHandler()->moduleExists('block') ? ['config:block_list']: []);
+

I couldn't go for a straight revert of the previous state, mergeTags was getting 3 arguments instead of 2. I think this solution is even more readable than the original solution.

Fixes the failures in FrontPageTest.

I updated the IS and added a beta evaluation. I think the beta evaluation is correct but I'm not sure.

Also fixed the nits from #72.

Committed/pushed to 8.0.x, thanks!