Problem/Motivation

The attach behavior for the horizontal tabs might get really slow depending on how much content will be under each horizontal tab. The problem is that the attach behavior uses the jquery .wrap() function which relies on the .appendChild() function. As an example in my case the horizontal tabs processing takes 2105ms! You might look at the following screenshot from the java script profiler.

Proposed resolution

1. Instead of modifying the DOM to wrap the content for the horizontal tabs in a outer div, just place the outer div in the twig template and there is no need for a DOM modification anymore.
2. Instead of modifying the DOM to insert the content for the tabs column, just place it in the twig template and there is no need for a DOM modification anymore.

Measurements for the total execution duration of the function Drupal.attachBehaviors:

The current approach:
10 subsequent reloads result in an average total execution time of 2189,8ms.

With the proposed resolution:
10 subsequent reloads result in an average total execution time of 1499,8ms.

Result:
An amazing execution speed up of 690ms!

Remaining tasks

Review.

User interface changes

none

API changes

none

Data model changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

Category: Feature request » Bug report
hchonov’s picture

Title: horizontalTabs attach behavior is slow » Optimize attach behavior Drupal.behaviors.horizontalTabs
Category: Bug report » Task
Issue summary: View changes
Status: Active » Needs review
FileSize
5.96 KB

Rethinking my previous proposal : we do not need a new implementation, but we still can get rid of .wrap and .before used in the attach behavior by placing this div elements directly into the twig template. By doing so and expecting them to be there I've achieved a really good performance optimization when having a really a lot of content.

Measurements for the total execution duration of the function Drupal.attachBehaviors:

Without the patch:
10 subsequent reloads result in an average total execution time of 2189,8ms.

With the patch applied:
10 subsequent reloads result in an average total execution time of 1499,8ms.

Result:
An amazing execution speed up of 690ms!

hchonov’s picture

FileSize
5.96 KB
973 bytes

Actually we could limit the search for the new elements directly to the first children.

aby v a’s picture

FileSize
575 bytes

Hi,

I have changed the comment with a full stop. Kindly check the patch.

nils.destoop’s picture

I'll check the patch. Did you also profile the vertical tabs? Horizontal tabs is a copy of vertical tabs. So i think they will have the same issue (unless they changed code since fieldgroup started)

hchonov’s picture

FileSize
6.37 KB

No, I haven't checked what is the current state of vertical tabs.

Re-rolling the patch as it could not be applied anymore.

cspitzlay’s picture

Issue tags: +Needs reroll

Any chance to get this optimization into 1.0 (once it is rerolled)?

hchonov’s picture

FileSize
6.06 KB

Re-roll...

Status: Needs review » Needs work

The last submitted patch, 9: 2760835-9.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review

So the only fail is where the whole branch is currently failing so not a problem with my patch.

campbellt_’s picture

Status: Needs review » Closed (outdated)

Closing due to outdated. Please reopen if still an issue.

DerryC’s picture

Status: Closed (outdated) » Needs review
Issue tags: -Needs reroll
FileSize
6.05 KB

Re-roll for 8.x-3.x...

idebr’s picture

Version: 8.x-1.x-dev » 8.x-3.x-dev
kfritsche’s picture

FileSize
6.17 KB

Re-roll.

nils.destoop’s picture

Thx for the patch and re-rolls. I added an extra visually-hidden on the list, as it would display an empty box when javascript is disabled.
I also switched to data attributes for the selectors.

nils.destoop’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.