Problem/Motivation

Drupal triggers behaviors on each Ajax changes. On the homepage, Drupal.behaviors.toggleEditMode.attach() called 13 times.

`Drupal.ajax.instances` defined inside the function and loop over on each call. It's about 70 loops in total on the homepage loading.

Proposed resolution

1. It doesn't reassign the value, we need not `filter` and then `forEach`. `filter` create a new ARRAY which uses extra memory.

2. Ideally, it should only execution after contextual links fully loaded or after Top left Toggle button is clicked.

3. Ideally, "fullscreen" should be returned from Ajax. (might be hack Drupal.Ajax to support data-progress-type)

4. Drupal has no 2-way data binding. If we can't do #3, I'd suggest changing the setting when you clicking the target edit area (here's what the #0 patch does)

Remaining tasks

- Review Patch

User interface changes

- NO

API changes

- NO

Data model changes

- NO

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

droplet created an issue. See original summary.

droplet’s picture

** Note: I don't familiar with OutSide_In :) And here's a quick patch in #0. I hope there's some better workaround :P

droplet’s picture

well. It can even more clean code

droplet’s picture

Priority: Normal » Major
Issue tags: +frontend performance

I raise the priority since `Drupal.attachBehaviors` reattached 9 times on homepage (default installation profile). A huge performance impact

Wim Leers’s picture

A huge performance impact

Can you quantify this?

tedbow’s picture

Status: Needs review » Needs work
+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -212,21 +213,6 @@
-          instance.options.data.dialogOptions.outsideInActiveEditableId = $(instance.element).parents('.outside-in-editable').attr('id');
-          instance.progress = { type: 'fullscreen' };

These 2 lines are not being replaced anywhere. We still need the behavior so I don't think this block of code can removed at all.

I manually tested and confirm that "active" block is not highlight with this patch. Which means we need a test for that since the patch passed.

drpal’s picture

  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -212,21 +213,6 @@
    -        .filter(instance => instance && $(instance.element).attr('data-dialog-renderer') === 'off_canvas')
    

    Can we also check for the existence of instance.options.data.dialogOptions.outsideInActiveEditableId to we can more strictly filter elements to avoid looking at potential duplicates?

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -212,21 +213,6 @@
    -          instance.progress = { type: 'fullscreen' };
    

    I think we need these.

droplet’s picture

Issue tags: +Needs JS tests
tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

tedbow’s picture

Issue tags: +Needs reroll
droplet’s picture

Issue tags: -Needs reroll

Remove reroll since current patch doesn't work very well :)

tedbow’s picture

I don't think we need the patch. I am not sure there is actually performance problem here.

From the Proposed solution:

2. Ideally, it should only execution after contextual links fully loaded or after Top left Toggle button is clicked.

We can't know when all contextual links are loaded. Ajax could add another part of the page when have new contextual links, correct?

Also the top edit button may never actually be clicked because click "Quick Edit" on any block will invoke edit mode

I don't see how we can not the event drupalContextualLinkAdded

3. Ideally, "fullscreen" should be returned from Ajax. (might be hack Drupal.Ajax to support data-progress-type)

Seems like be a BC break now it would be using the default progress type for jQuery

drpal’s picture

Version: 8.4.x-dev » 8.5.x-dev
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.23 KB
77.19 KB
249.03 KB

Alright. I'm not sure this is even needed anymore, but this is mostly a micro optimization.

This function, prepareAjaxLinks() appears to be fired 11 times via contextual.js

The slowest part of this is the jQuery .parents() lookup.

Wim Leers’s picture

A huge performance impact

vs

this is mostly a micro optimization

droplet’s picture

Quick response to #14

I didn't measure the time diff anyway. It's not so noticeable to human but developer would care. Time also affected by each behavior execution time. We have no long-running behaviors in CORE.

Before Patch:

simple math:
Drupal.behaviors attaches * trigger buttons number + re-attach trigger another round Drupal.behaviors attaches * trigger buttons number

more accurate:
I can't work out for it now and don't think really needed.
So with my original issue summary above, it was called 13 times and end up with 70 loops (It was logging with console.log)

On the AVG, the pages called Drupal.behaviors 30 times (I remembered nod_ was doing a roughly researching on this part somewhere)

70 / 13 * 30 ~= 160 loops. (actually it should be a non-linear equation)

After Patch:
my suggestion will turn into constant, single time.

Best case: no clicks, zero execution. 1 click, 1 execution. (It's not the Drupal-way. In Drupal, we always initialize everything at page load)
Better case: Drupal.behaviors attaches * trigger buttons number

** It may not correct with up-to-date code. I will re-evaluate it for sure! Thanks ALL!!

tedbow’s picture

If #13 approves the performance I am all for it. It doesn't need some the larger changes that were suggested in the proposed solution in the issue summary. Those other changes would require changes outside of this module I think.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.