Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We noticed that attachment merging is suboptimal. Something like the attached patch should be faster. Needs work to pass tests and profiling to prove that it's faster.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2471228-mergeAttachments-complete-22.patch | 14.68 KB | jcnventura |
#22 | 2471228-mergeAttachments-tests-22.patch | 13.6 KB | jcnventura |
#22 | interdiff.txt | 13.47 KB | jcnventura |
#7 | 2471228-mergeAttachments-7.patch | 1.09 KB | Hjarnmastara |
ddd-profiling-attachment-merging.patch | 1.11 KB | Wim Leers | |
Comments
Comment #1
Wim LeersComment #3
mr.baileysAssigning to myself to work on this during DDD Montpellier.
Comment #4
mr.baileysUnassigning, the specifics of the array merging going on here are way above my head.
Comment #5
Wim LeersHm, sorry to hear that! :( That means this is definitely not a Novice issue, if even mr.baileys is bailing out.
Comment #6
Hjarnmastara CreditAttribution: Hjarnmastara at Calibrate commentedComment #7
Hjarnmastara CreditAttribution: Hjarnmastara at Calibrate commentedTests 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.
Comment #8
Wim LeersCan you provide the 3v4l links? Thanks :)
Assigning to Fabianx for review.
Comment #9
Hjarnmastara CreditAttribution: Hjarnmastara at Calibrate commentedI used this: http://3v4l.org/ukJiQ
Please take into consider that this was my first time with profiling.
Cheers
Comment #10
Hjarnmastara CreditAttribution: Hjarnmastara at Calibrate commentedI will work on a bit better 3v4l.org link (test case) after food time!
Comment #11
Hjarnmastara CreditAttribution: Hjarnmastara at Calibrate commentedWithout 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
Comment #12
Fabianx CreditAttribution: Fabianx for Acquia commentedI would like to see some XHProf profiling for this.
The current change makes me uncomfortable as we now use, use mergeDeepArray() all the time?
Comment #13
jcnventura CreditAttribution: jcnventura commentedI'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.
Comment #14
jcnventura CreditAttribution: jcnventura commentedDuplicate
Comment #15
Fabianx CreditAttribution: Fabianx for Acquia commentedIndeed, we have only library and html_head left, which are both not deep nested ...
Lets see if tests pass ...
Comment #17
jcnventura CreditAttribution: jcnventura commentedYes, 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?
Comment #18
jcnventura CreditAttribution: jcnventura commentedComment #19
Fabianx CreditAttribution: Fabianx for Acquia commentedI 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?
Comment #20
jcnventura CreditAttribution: jcnventura commentedFabianx. 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.
Comment #21
Wim LeersIf we'd make
\Drupal\system\Tests\Common\MergeAttachmentsTest
comprehensive, we could justify that.Comment #22
jcnventura CreditAttribution: jcnventura commentedThat 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.
Comment #23
Wim LeersAwesome! :)
:D :D
Comment #24
Fabianx CreditAttribution: Fabianx for Acquia commentedNooooooooo ... 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
Comment #25
catchFixed on commit.
Committed/pushed to 8.0.x, thanks!