Improve performance of the user.permissions.js script running in /admin/people/permissions.

This is great.

I am just updating tags according to the issue tag guidelines → .

I just attached screenshots of my Firefox browser running the project I mentioned with the Javascript performance audit script, both on the original code (403ms) and the new code (12ms).

I did some manual testing on this.

I created 15 custom roles and added all permissions to authenticated (lol).
Stock 11.x branch running a console.time on the permissions page.

Range 55-75ms (most often was between 66 and 72)

Checking out this fork and same same script timed.

Range 36-46 (most often was between 42 and 46 ms)

Worst case performance improvement in this case is 30% while the best case improvement is: 50%

Code looks good. I wonder if requestAnimationFrame warrants a comment on what you're doing there.

I think someone that is more familiar with JS needs to review to approve this.

Two quick questions:

requestAnimationFrame does that allow each checkbox in a row to be processed before repainting?
on the intersectionObserver what happens if you save before it finishes processing? I assume nothing since the checked state still exists and this is just setting up the dummy checkboxes for authenticated right?

@nicxvan: when the permissions page is loaded, I am immediately running the script to activate the dummy checkboxes on the checked columns of Authenticated user.

This will for sure cause the script to slow down the page in a situation where you are checking all checkboxes in the Authenticated user column because the code will run more or less like the previous version where all rows were being processed upon page load.

In my experience, though, that is something we don't see in real life (authenticated users being granted most of the permissions).

You bring up a point that makes me think further: why should I replace those checkboxes when the page is loaded? Why not simplify the script even further and let IntersectionObserver do the magic for all rows, including the pre-checked rows?

I think I will remove this preprocessing of the authenticated rows, and use a one-rule-fits-all using IntersectionObserver. The further performance gain in this case will be minimal, but the code will be even leaner.

requestAnimationFrame will 'schedule' the repaint associated with activating/deactivating the checkboxes to the next repaint.

Changes look great to me!

Updated the issue queue description to reflect the latest changes I had applied to simplify the code even further.

Thanks it looks very promising, a few things in the MR, I'm attaching a piece of code for inspiration, it's not cleaned up and we can remove some nesting but it's to give some direction on where I see this going.

I tested the patch and the performance has been improved a lot.

Without the patch the script take 51.7ms to run.

With the patch became 1.4ms.

Nice!

Went with some CSS to manage show/hide of the checkboxes

Thank you @_nod and @joaopauloc.dev for the updates and tests on this ticket, I am happy to see my initial effort seems fruitful. I can't test the latest changes by @_nod right now, but will try to do so next week.

I fixed the linting issue there is a failure here:

1)
Drupal\Tests\user\FunctionalJavascript\UserPermissionsTest::testPermissionCheckboxes
Error: Call to a member function isVisible() on null

/builds/issue/drupal-3464530/core/modules/user/tests/src/FunctionalJavascript/UserPermissionsTest.php:66

ERRORS!
Tests: 1, Assertions: 5, Errors: 1.

I'm guessing the isVisible method has some trouble with the CSS managing the visibility. Might need to write some JS that checks "manually" the visibility of the element.

Change look great! Intersection observer and css were both great ideas.

I tested again and it's a huge performance improvement.

I like the rename from dummy to fake as well.

My only code contribution was removing an extra line so that tests would run so I think it's ok for me to mark RTBC.

@_nod, the changes you are introducing are brilliant, they improve the code even further. My only stylistic comment has to do with the name of the IntersectionObserver variable. Since we are now observing checkboxes, not table rows, I think the variable should be renamed accordingly.

renamed the variable and reduced the overall nesting depth

I tested again with just a console time with the same conditions (15 roles).

Stock 11.x the timer gets called 5 times:

test: 62ms - timer ended
test: 1ms - timer ended
test: 0ms - timer ended
test: 0ms - timer ended
test: 1ms - timer ended

This fork it gets called 4 times, but says the timer exists 3 times:

test: 3ms - timer ended
Timer “test” already exists.
Timer “test” already exists.
Timer “test” already exists.

Also just checking the page load speed it saves about 250ms to load the page entirely.
1.05 to 1.11seconds on 11.x
830 to 910ms with this fork

Assuming the concurrent calls are ok then I think RTBC is the correct status unless we want to rename: CheckedCheckboxObserver to checkedCheckboxObserver.

renamed the variable, thanks for the performance tests :)

I think it's good to go, I confirmed all the comments have been taken care of.

Hi,

This is a great improvement! I did a test with a website with 4 roles and some modules. The permissions table has 535 rows.

The results are:

Before: 91.7ms
After: 1.7ms

Hopefully this will be merge as soon as possible.

Thank you, this is working!
I have created a patch file from the MR.

nod_ → credited aaronbauman → .

nod_ → credited catch → .

nod_ → credited droplet → .

nod_ → credited geerlingguy → .

nod_ → credited quietone → .

nod_ → credited sun → .

Porting credit from 🐛 Improve performance of /admin/people/permissions Needs work

nod_ → closed merge request !8974

Worked on this but we're low on JS reviewers so I'm committing this one.

Backporting to 10.3.x because of the potential performance improvements

Committed and pushed b80bfda19d to 11.x and b586e9027c to 11.0.x and 04d9ff136f to 10.4.x and 3d9ddf678d to 10.3.x. Thanks!

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