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
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3521884-with-patch.png | 162.95 KB | nigelcunningham |
| #9 | 3521844-without-patch.png | 158.55 KB | nigelcunningham |
| core-tableresponsive-speed.patch | 1.27 KB | nigelcunningham |
Issue fork drupal-3521884
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
Comment #2
nigelcunningham commentedComment #3
nigelcunningham commentedComment #4
smustgrave commentedThanks for reporting, issues need to be in MRs against 11.x
Probably need some profiling to be included, or a performance test.
Comment #5
catchThe 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.
Comment #6
smustgrave commentedSo just some profiling numbers to the ticket then should work.
Comment #7
nigelcunningham commentedOk. I identified the problem from profiling so that should be no problem at all. I'll add this in a little while.
Comment #8
nigelcunningham commentedOh, 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.
Comment #9
nigelcunningham commentedHere are screenshots without the patch:
and with it:
(A reduction from 94s to 2s in my instance).
Comment #10
catchFor the 404 on creating a new branch - are you definitely logged into gitlab? Sometimes d.o and gitlab sessions go out of sync.
Comment #11
nigelcunningham commentedThanks, 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.
Comment #12
nigelcunningham commentedThe 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.
Comment #13
smustgrave commentedCan you open the MR for the tests to run please
Comment #15
smustgrave commentedActually just did it real quick
Comment #16
smustgrave commentedProfiling looks good. MR shows there are eslint that needs to be addressed
Left 1 comment in the MR
Comment #17
nigelcunningham commentedHmm, a test fails but it seems to be unrelated:
https://git.drupalcode.org/issue/drupal-3521884/-/jobs/5128162
Comment #18
nod_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 :)
Comment #19
nicxvan commentedI wonder how much this will help when you have test module discovery on.
Comment #20
nigelcunningham commentedFeel 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.
Comment #21
nod_updated the code, can you check it solves the problem?
Comment #22
catchJust 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.
Comment #23
catchThere'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.
Comment #29
nod_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!