Optimize merging of attachments

Assigning to myself to work on this during DDD Montpellier.

Unassigning, the specifics of the array merging going on here are way above my head.

Tests should pass, profiled a bit at the website 3v4l.org, just saw a very small improvement but I don't have any more inspiration for now.

I used this: http://3v4l.org/ukJiQ
Please take into consider that this was my first time with profiling.
Cheers

I will work on a bit better 3v4l.org link (test case) after food time!

Without patch: http://3v4l.org/2PgFh/perf#tabs
With patch: http://3v4l.org/Q4qEn/perf#tabs

I believe my patch will only have a small impact when the 'drupalSettings' key is present in both arrays that we wish to merge. Anyway, it was fun trying to improve this function.

Cheers

I would like to see some XHProf profiling for this.

The current change makes me uncomfortable as we now use, use mergeDeepArray() all the time?

I've modified Hjarnmastara's patch a bit, so that we get the desired performance boost.

According to 3v4l, it uses only about 50% of the previous user time (http://3v4l.org/4ht5i/perf#tabs). I did a few simpletest tests on my rig, but I need to see the results from the official bot.

Duplicate

Indeed, we have only library and html_head left, which are both not deep nested ...

Lets see if tests pass ...

Yes, those fails made sense.. I was able to get a few of those test failures to pass with this improved patch that uses array_merge_recursive instead of array_merge. Still only about 50% of before in 3v4l (http://3v4l.org/bRiOX/perf#tabs)

@Fabianx are there any instructions on how to provide the xhprof data?

I am okay with 3v4l with latest patch now, its obvious native code is faster.

However:

https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Nest...

Can we justify why we no longer need to use mergeDeep() instead of array_merge_recursive?

Fabianx. we can justify it according to https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup... by the fact that all the allowed values for the #attached property must be arrays, so array_recursive_merge will result in a correct merge in this case.

That was a lot more work than I expected, as \Drupal\system\Tests\Common\MergeAttachmentsTest was testing for stuff that doesn't exist anymore.. It should be very comprehensive now. And I made sure to add some kittens.

And I made sure to add some kittens.

Nooooooooo ... OK :-D

Fortunately I just added some more llama's :-p.

--

The patch looks great to me, the test coverage was extended extensively and it is absolutely clear that a native array_merge_recursive is always faster than our own ...

Therefore => RTBC

+++ b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
@@ -195,4 +167,246 @@ function testJsSettingMerging() {
+//    $this->assertIdentical($expected['#attached'], $renderer->mergeAttachments($b['#attached'], $a['#attached']), 'Attachments merged correctly; opposite merging yields opposite order.');

Fixed on commit.

diff --git a/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
index b430ee6..466ed3a 100644
--- a/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
+++ b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
@@ -295,8 +295,7 @@ function testHtmlHeadMerging() {
         'system_meta_content_type',
       ),
     );
-//    $this->assertIdentical($expected['#attached'], $renderer->mergeAttachments($b['#attached'], $a['#attached']), 'Attachments merged correctly; opposite merging yields opposite order.');
-    $this->assertIdentical($expected['#attached'], $renderer->mergeAttachments($b['#attached'], $a['#attached']));
+    $this->assertIdentical($expected['#attached'], $renderer->mergeAttachments($b['#attached'], $a['#attached']), 'Attachments merged correctly; opposite merging yields opposite order.');
   }
 
   /**

Committed/pushed to 8.0.x, thanks!