Problem/Motivation

tableresponsive.js is used on the extensions page and elsewhere where tables are loaded. If a page has multiple tables (eg the extensions page with multiple module categories), the eventhandlerEvaluateColumnVisibility event handler will be invoked (number of tables added so far) times (for a page with 50 tables, 50 + 49 + 48... times).

Steps to reproduce

Open /admin/modules. Open the developer console of your web browser and find and set a breakpoint in the eventhandlerEvaluateColumnVisibility function. Reload the page and observe that the function is called repeatedly. If you also set a breakpoint at the TableResponsive.tables.push call in the behaviour function, you'll see that the event handler is called once after the first push, twice after the second and so on.

Proposed resolution

Add a flag to the top of the evaluate function, allowing the event handler to exit immediately before the last table is added. Set the flag during the attach call when we aren't on the last table being attached.

Remaining tasks

I have a working patch and have confirmed on my local that the page load time for my site that has 50 module categories goes from 30s plus to completing almost immediately. I have compared the page before and after applying the patch and have not found any differences.

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

Not required

Issue fork drupal-3521884

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nigelcunningham created an issue. See original summary.

nigelcunningham’s picture

Status: Active » Needs review
nigelcunningham’s picture

Issue summary: View changes
smustgrave’s picture

Version: 10.4.x-dev » 11.x-dev
Status: Needs review » Needs work

Thanks for reporting, issues need to be in MRs against 11.x

Probably need some profiling to be included, or a performance test.

catch’s picture

The steps to reproduce in the issue summary are sufficient for profiling.

Unless this information is included in the chrome performance log, which I don't think it is, it would not be possible to add performance test support for it, and we definitely don't have that available now so shouldn't block a fix anyway.

smustgrave’s picture

So just some profiling numbers to the ticket then should work.

nigelcunningham’s picture

Ok. I identified the problem from profiling so that should be no problem at all. I'll add this in a little while.

nigelcunningham’s picture

Oh, I've been reminded why I didn't add an 11.x branch: When I click on the "Create new branch" above (https://git.drupalcode.org/issue/drupal-3521884/-/branches/new?branch_na...), it's a 404 for me.

nigelcunningham’s picture

StatusFileSize
new158.55 KB
new162.95 KB

Here are screenshots without the patch:

Screenshot without patch applied

and with it:

Screenshot with patch applied.

(A reduction from 94s to 2s in my instance).

catch’s picture

For the 404 on creating a new branch - are you definitely logged into gitlab? Sometimes d.o and gitlab sessions go out of sync.

nigelcunningham’s picture

Thanks, I wasn't aware of that.

For anyone else searching for a solution in future, I could tell I wasn't logged into Gitlab by going to https://git.drupalcode.org/. In the top right corner, it had a 'Sign in' link. I was already signed in on D.O so just needed to click that button. It then showed my profile and the sign in button disappeared.

nigelcunningham’s picture

Status: Needs work » Needs review

The patch applies without modification to 11.x-dev. A branch has been created and pushed and profiling screenshots supplied above; resetting to needs review in the belief that I've done everything that was wanted.

smustgrave’s picture

Can you open the MR for the tests to run please

smustgrave’s picture

Actually just did it real quick

smustgrave’s picture

Status: Needs review » Needs work

Profiling looks good. MR shows there are eslint that needs to be addressed

Left 1 comment in the MR

nigelcunningham’s picture

Hmm, a test fails but it seems to be unrelated:

https://git.drupalcode.org/issue/drupal-3521884/-/jobs/5128162

nod_’s picture

The very minimum is to move the .trigger(resize) code out of the tableresponsive constructor. Add that after the once() in the attach behavior and it should mostly fix the problems of runaway initializations. We can also add a debounce to prevent too many calls to that function. The resize event is explicitly mentioned on the debounce script as a valid use case :)

nicxvan’s picture

I wonder how much this will help when you have test module discovery on.

nigelcunningham’s picture

Feel free, _nod.

I have to admit I'm not following what you're thinking so it might be better if you just go ahead and implement it.

nod_’s picture

Status: Needs work » Needs review

updated the code, can you check it solves the problem?

catch’s picture

Just reproduced this on Drupal CMS - page freezes for about three seconds and chrome performance profiling pinned it down to tableresponsive.js, then searched and reached there again.

catch’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Related issues: +#2277761: Remove unnecessarily complex logic from tableresponsive.js

There's also #2277761: Remove unnecessarily complex logic from tableresponsive.js.

This is very straightforward. We can try to refactor things in other issues, but stopping the page from completely locking up great.

  • nod_ committed cb18f9b4 on 10.5.x
    Issue #3521884 by nigelcunningham, nod_, smustgrave, catch:...

  • nod_ committed 4f50dadb on 10.6.x
    Issue #3521884 by nigelcunningham, nod_, smustgrave, catch:...

  • nod_ committed 07688803 on 11.1.x
    Issue #3521884 by nigelcunningham, nod_, smustgrave, catch:...

  • nod_ committed 4a461032 on 11.2.x
    Issue #3521884 by nigelcunningham, nod_, smustgrave, catch:...

nod_’s picture

Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 27ad23e01d2 to 11.x and 4a461032dec to 11.2.x and 076888031a9 to 11.1.x and 4f50dadbcb8 to 10.6.x and cb18f9b4fdb to 10.5.x. Thanks!

  • nod_ committed 27ad23e0 on 11.x
    Issue #3521884 by nigelcunningham, nod_, smustgrave, catch:...

Status: Fixed » Closed (fixed)

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