On a site with ~20 content types, and quite a few modules, there may be a few hundred permissions at /admin/people/permissions. In this case, the web-page loads relatively quickly (network) but then flatlines the CPU at 100% for a long time (70 seconds on my 2.4GHz Macbook Pro, Google Chrome). Roughly same results in Safari and Firefox.

I will use Speed Tracer http://code.google.com/webtoolkit/speedtracer/ to investigate this a bit further, but initial results in attached screenshot.

Files: 
CommentFileSizeAuthor
#48 Speed Tracer D7.9.jpg112.96 KBlyricnz
#47 drupal.user-permissions-js.47.patch3.89 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 34,110 pass(es). View
#27 Speed Tracer.jpg181.8 KBlyricnz
#28 Speed Tracer-1.jpg168.88 KBlyricnz
#25 drupal.user-permissions-js.25.patch4.71 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,776 pass(es). View
#21 drupal8.user-permissions-js.21.patch4.74 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,793 pass(es). View
#18 drupal8.user-permissions-js.18.patch4.21 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 32,773 pass(es). View
#14 Speed Tracer-4.jpg151.27 KBlyricnz
#13 Speed Tracer-3.jpg188.01 KBlyricnz
#10 drupal8.user-permissions-js.10.patch4.06 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,232 pass(es). View
#9 drupal8.user-permissions-js.8.patch3.96 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,223 pass(es). View
#8 Speed Tracer-2.jpg123.62 KBlyricnz
#7 drupal8.user-permissions-js.7.patch2.75 KBsun
PASSED: [[SimpleTest]]: [MySQL] 32,214 pass(es). View
#2 Speed Tracer-1.jpg57.07 KBlyricnz
#2 Home | Cave Divers Association of Australia.jpg213.66 KBlyricnz
Speed Tracer.jpg184.68 KBlyricnz

Comments

catch’s picture

Subscribe, quality screenshot.

lyricnz’s picture

The problem is basically that user.permission.js adds/toggles checkboxes all across that page, each of which causes a ~50ms layout operation. See attached stack trace (hundreds are the same), and source code.

miro_dietiker’s picture

subscribing.
Indeed the permission table is something that grows continuously till it's blocks everything...

The screenshots are a very nice starting point.

xjm’s picture

pillarsdotnet’s picture

Version: 8.x-dev » 7.2
Status: Needs review » Active
Issue tags: -needs backport to D7

Proposed fix:

  1. Back out changes in #78941: Usability: Auto-check permissions if "authenticated" has them
  2. The form building function should ensure that each implied checkbox is checked.
  3. Implied checkboxes should be rendered with a hover-popup saying "Implied by XXX" where "XXX" is the checkbox that implied it.
  4. When an implied checkbox is unchecked, Javascript should uncheck the box that implied it, then hide the hover popups for that row.
  5. When "anonymous" or "authenticated" checkbox is unchecked, javascript should uncheck all implied checkboxes, and hide the hover popups for that row.
  6. The validation function should do the same as the Javascript, in case javascript is turned off.
  7. The validation function should then uncheck each box that is (still) implied, before saving data.
xjm’s picture

Version: 7.2 » 8.x-dev
Issue tags: +needs backport to D7

Moving to 8.x and tagging for backport.

sun’s picture

Status: Active » Needs review
FileSize
2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 32,214 pass(es). View

Test again with attached patch, please?

Further performance optimizations are possible, these are the most obvious and simplest fixes.

lyricnz’s picture

FileSize
123.62 KB

Sorry @sun - no big change. Here's a stack trace of the problematic method, from an uncompressed jquery.

FWIW, if I hook_form_alter() the JS off the user_admin_permissions form, the page loads in a couple of seconds, rather than about 70.

sun’s picture

FileSize
3.96 KB
PASSED: [[SimpleTest]]: [MySQL] 32,223 pass(es). View

Hence, attached patch should fix it.

Sometimes, there's no other way than to circumvent jQuery. I ran into similar performance issues in some contrib projects too already.

sun’s picture

FileSize
4.06 KB
PASSED: [[SimpleTest]]: [MySQL] 32,232 pass(es). View

Sorry, .triggerHandler() is incompatible here. Fixed that and explained in a comment.

lyricnz’s picture

@sun #10 is about ten times faster than the current code, and looks okay visually. Still takes ~6s here with ~280 permissions and 7 roles (don't ask).

sun’s picture

Version: 7.2 » 8.x-dev
Status: Active » Needs review
Issue tags: +needs backport to D7

6 seconds still sounds like too much. Though the pure rendering of 1,960 table cells and checkboxes probably takes a second on its own already. Do you have new details on where the time is spent?

lyricnz’s picture

FileSize
188.01 KB

Here is updated Speed Tracer. It's spending the same amount of time as before doing the JS callbacks (5.9s) but now only 0.2s doing layout. Hard to tell exactly what it's doing: the line that's actually being reported as consuming the time is jquery-1.4.4-uncompressed.js::[unknown]() Line 873:

871	// Cleanup functions for the document ready method
872	if ( document.addEventListener ) {
873	 DOMContentLoaded = function() {
874	 document.removeEventListener( "DOMContentLoaded", DOMContentLoaded, false );
875	 jQuery.ready();
876	 };

I'll have a bit more of a look now.

lyricnz’s picture

FileSize
151.27 KB

The problem is ultimately that we're adding 1300 checkboxes to an active DOM. Hmm, a quick hack here, and it looks detaching the table from the DOM, performing all the modifications, then re-adding it - takes 2.7 seconds instead of 5.9s:

      var $table = $(this);
      var $parent = $table.parent();
      $table.detach();
      $table.find(':checkbox:not(.rid-2,.rid-1)').addClass('real-checkbox').each(function () {
        $dummy.clone().insertAfter(this);
      });

      // Initialize the authenticated user checkbox.
      $table.find(':checkbox.rid-2')
        .bind('click.permissions', self.toggle)
        // .triggerHandler() cannot be used here, as it only affects the first
        // element.
        .each(self.toggle);
      $parent.append(this);

I'm not a JS guy, so I may be doing something totally crazy here...

lyricnz’s picture

BTW, I am currently benchmarking a few aspects of the code used in this code:

:checkbox vs [type=checkbox]
:not vs .not()
.insertAfter() of an object vs html
detached table vs attached
lyricnz’s picture

(Using patch in #10 as a baseline. Results are for Chrome+Firefix 5)

1) :checkbox vs input:checkbox vs [type=checkbox] vs input[type=checkbox]

input[type=checkbox] is about 20 times faster than :checkbox that we're using.
see http://jsperf.com/big-checkbox-test/2

2) ':not(x)' vs .not(x)

.not() is about twice as fast as :not
http://jsperf.com/big-checkbox-test/3

3) meh, maybe later

4) detached table vs attached

For a large DOM, detached is about twice as fast - see #14

lyricnz’s picture

$('input[type=checkbox]').hasClass('.rid-2');

vs

$('input[type=checkbox].rid-2');

the latter is 15% faster (duh :)

lyricnz’s picture

FileSize
4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 32,773 pass(es). View

Here's a patch that:
- based on patch from @sun in #10
- detach the table from the dom before manipulation
- replace :checkbox selectors with input[type=checkbox]
- replace :not selectors with .not()

The JS completed in 2.4 seconds on the same machine as quoted previously.

fubhy’s picture

I am just thinking here... But... Wouldn't it make sense to introduce some kind of maximum number for which we keep on using the full-list and when we breach that number (like 500 rows or so) we instead display a Select Dropdown which lists all the permission categories and then on select we print the list of that permission categories with AJAX? There could still be a button to "show all". Or it could be a multi-select so the user can choose more categories to display at once.

lyricnz’s picture

Whether that's the right solution or not, we certainly do need a better UI design for this page. Suggest you open a new issue? This one is kindof an emergency fix for catastrophic performance, rather than fixing the UI.

sun’s picture

FileSize
4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 32,793 pass(es). View

Thanks @lyricnz! Very inspiring :)

I've fixed some trailing white-space, added comments, and also fixed the bogus re-insertion of the table by dynamically using .prev() with .after() or .parent() with .append().

I think this patch is ready to fly now.

lyricnz’s picture

Status: Needs review » Reviewed & tested by the community

Works well for me.

catch’s picture

Issue tags: +Performance

Tagging. I am no good at js but have been following via irc and the issue, and this is really nice work. The summary is that with lyricnz's test case, time spent is down from 65 seconds to 3.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

NICE job on this!! Holy cow.

Committed and pushed to 8.x and 7.x!

sun’s picture

Status: Fixed » Needs review
FileSize
4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 32,776 pass(es). View

Here's the alternative, dynamic on-demand approach we briefly discussed in IRC.

It will probably work fast on @lyricnz' sample. Less so on sites that actually have the auth users checkbox ticket for almost all roles. So kinda use-case dependent - although I think that @lyricnz' sample is more common, especially due to the new admin role feature in D7, but also, because I'd imagine sites granting lots of permissions to auth users to be smaller in general (i.e., having less roles and permissions in the first place).

Nevertheless, since the committed code is quite a major workaround for performance, it would be a good idea to investigate, benchmark and compare this alternative approach.

sun’s picture

@lyricnz: Any chance to test #25? I think the on-demand approach superior.

lyricnz’s picture

FileSize
181.8 KB

@sun: Slower (3.4s vs 2.4s with previous patch). Caused by 58 layouts, caused by the DOM being attached during the toggle.

Edit: attached screenshot shows a Layout caused by something else.

lyricnz’s picture

Status: Needs review » Needs work
FileSize
168.88 KB

Reinstating the detach/reattach code reduced time to 2.0

somatics’s picture

I don't think this is exclusively a Drupal 8 issue: I am encountering what I think is the same issue described here, but on my Drupal 7 site with a lot of permissions and roles. Disabling javascript in my browser "fixed" it entirely. I've created an issue for it in the Drupal 7 issue queue:

http://drupal.org/node/1214250

webchick’s picture

Marked the other issue as duplicate, with explanation.

mdupont’s picture

somatics’s picture

I relieved to hear this is a known problem that is being fixed. Thanks so much for all the hard work!

I was at a loss for the past month, and concerned it was just some undiagnosable mystery within our own data. I am having only two other issues that are like this, and now I am wondering if they are a similar system problem -- remember we have a lot of modules installed, so perhaps it is pushing the performance boundaries:

Two other pages timeout on this site:

The /update.php page and the /admin/config page. Keep in mind that on this site, updates can be run from Drush without a hiccup, and all sub pages listed on the config page can be accessed without a single problem -- and the /admin/index page, which lists all the same things that /admin/config does (and more), also displays just fine.

So, I don't want to muddy the waters here, but just in case this was a related javascript issue or something like that, I wanted to mention it. Do those pages have any major javascript activity or anything else that might be caused by this same issue?

lyricnz’s picture

Raise a new issue - this one relates specifically to the /admin/people/permissions page.

geerlingguy’s picture

Subscribe, and wow! What a difference the already-applied patch makes. I was lamenting ever visiting the permissions page, with 200+ permissions (we have profile2 set up in a funky way that ends up adding a new set of permissions for each of our site's 'networks', so this number will only increase).

BenK’s picture

Subscribing

fangel’s picture

Just FYI, if you're using LoginToboggan the above patch won't do anything for you, because that module replaces user.permissions.js with it's own implementation.
I've created the issue #1290730: With large number of permissions /admin/people/permissions becomes unusable on the LoginToboggan issue-queue to try and make sure LoginToboggan updates its code as well (or better yet, find a way to perform its changes without having to reproduce the entire script)

hunmonk’s picture

keeping in line with the commit in #24, i've pushed a matching fix to logintoboggan 7.x-1.x-dev.

sun’s picture

So if I get #28 right, then #25 improves browser rendering performance by another 33%.

Re-roll, commit?

lyricnz’s picture

Well, it reduced the time from 2.4s to 2.0s (17% improvement).

geerlingguy’s picture

My test results for patch in #25 (on a huge permissions page, as mentioned above, due to a bunch of Profile2 profile types):

  • Before patch: 39.1s (almost the entire time with 1 processor core at 100%, but it took about 7s for the initial page to display)
  • After patch: [gave up] (I let the clock run until 2 minutes, with processor at 100% the whole time, and even dismissed two Chrome 'this page is hanging' dialogs).

Both times I opened a new incognito window (to be sure JS wasn't cached), flushed all caches on a locally-hosted drupal 7 site, and started the stopwatch at the moment I clicked 'Permissions'.

I repeated the test twice, to make sure my patching was correct (git reset --hard, then git apply -v [patchname]).

xjm’s picture

Hmm, so that sounds to me like the patch is not actually a performance improvement in all cases. (To put it mildly.)

lyricnz’s picture

@geerlingguy: how many roles * permissions do you have, that this page takes 39 seconds to run the JS?

geerlingguy’s picture

After doing a search for <td class="checkbox"> on the permissions page source, I found 6,540 checkboxes, and I have 6 roles, so there were 1,090 individual permissions to go through.

One thing that would speed up my particular site would be hiding the entire 'module-profile2' section (there are over 5,000 checkboxes just in that section of the permissions page, owing to the fact that I have a profile type per 'network' on my site, and there are over 200 networks...). Could I simply hide that section in a hook_page_alter or something like that? Might just do that...

geerlingguy’s picture

Okay, I've added the Filter Permissions module (found it was doing exactly what I was about to do, except in a form_alter), and the results are now (without displaying Profile2 permissions):

  • Before patch: 10.0s
  • After patch: 6.9s

Average of three tests each, first two clearing browser caches, third test a simple Command-R refresh (but all three were within .1s).

This is with around 1,200 permissions checkboxes on the page.

astutonet’s picture

Hi friends.

I have this same issue on D7.9 version.

I did a test with the patch in # 25, but didn't work. The error is:

Fatal error: Unsupported operand types in C:\wamp\www\drupal\modules\user\user.admin.inc on line 696

On line 696 of user.admin.inf file, we have this code:

);

As this issue discusses a problem in a different version, I would like to know from friends if the patch above is applicable to my version.

If not, I wonder if there one version of the patch wich applies to the current version of Drupal, because I searched and didn't find.

Thank you.

lyricnz’s picture

@astutonet:
Drupal 7.9 already includes the patch, it was applied on July 3 #24, and is in Drupal 7.6 or newer.

Don't use the patch in #25 as-written. As commented in #28, it's actually slower (at least in my case), due to it performing DOM manipulations on an attached DOM. I re-added the detach code, and *that* improved performance - not #25 itself. I haven't rerolled the patch with the fix yet.

lyricnz’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
PASSED: [[SimpleTest]]: [MySQL] 34,110 pass(es). View

Here's an updated version of #25 that reinstates the detach from the DOM.

lyricnz’s picture

Status: Needs review » Needs work
FileSize
112.96 KB

Patch needs fix - detach not working work.

nod_’s picture

Issue tags: +JavaScript

Don't mind me just tagging, too interesting to let this alone :)

aaronbauman’s picture

For anyone coming to this thread for a quick fix, here's the solution i used to get rid of invisible elements and dummy checkboxes altogether:

function mymodule_form_alter($form, $form_state, $form_id) {
  if ($form_id == 'user_admin_permissions') {
    drupal_add_js('jQuery(document).ready(function() { 
      jQuery(".element-invisible").remove();
      jQuery(".dummy-checkbox").remove();
     });', 'inline');
  }
}

Of course this doesn't resolve the initial pageload and render time, but it at least makes the page usable after load.

Also, this will make control-f (find on page) work again by removing the invisible elements.
I guess these are meant as an accessibility feature, but they significantly degrade performance on a medium-sized site even on my brand new Mac Mini.

This hack might work for D8 too - i haven't tested.

tierso’s picture

Version: 8.x-dev » 7.19

Having the same problem, and even before reaching this point, I thought the permissions tab was a very lacking (bloated) feature. It might work out fine for a few roles and modules, but once the site "grows" it gets way out of hand.
Why are there no filters (can't even find a single module to add this function, Like the module filter for instance) or even better. Start with a page providing options to select what permissions to view, based on permission category and/or roles, and then display what the admin wishes to see.

pjcdawkins’s picture

Version: 7.19 » 8.x-dev

@tierso http://drupal.org/project/filter_perms will solve your problem temporarily.

tierso’s picture

Ah, my dream coming true. Thank you!
It has everything I could ever wish for.

attiks’s picture

Issue summary: View changes

FYI: we ran into similar problems a lot, so I've uploaded a cleaned up version of our module https://www.drupal.org/project/dream_permissions. For the moment D7, but can not be hard to port to D8

droplet’s picture

Is it still a problem on modern browsers ?

attiks’s picture

It sure is, on one project the permissions page ended up having 32.000 checkboxes and a loading time of 20+ sec. The dummy checkboxes almost double the number of checkboxes, in our javascript we just disabled the real checkboxes. The only way so solve the loading time is to load less and where possible load it using json.

aaronbauman’s picture

Confirming, yes, loading 4+MB of HTML and running inefficient javascript on all of it is still a problem on late-2015 hardware.

catch’s picture

I've also seen the permissions page get big enough that the POST goes over max_input_vars, which then silently truncates the variables. Adding #1565704: Core interfaces can go over max_input_vars as a related issue.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 18cb277 on 8.3.x
    Issue #1203766 by sun, lyricnz: Fixed With large number of permissions /...

  • webchick committed 18cb277 on 8.3.x
    Issue #1203766 by sun, lyricnz: Fixed With large number of permissions /...
SKAUGHT’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 18cb277 on 8.4.x
    Issue #1203766 by sun, lyricnz: Fixed With large number of permissions /...

  • webchick committed 18cb277 on 8.4.x
    Issue #1203766 by sun, lyricnz: Fixed With large number of permissions /...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.