node_access_grants()
is called once per node during a request when node_access is implemented. For most implementations, this is a very cheap request. When implementations of hook_node_access_grants_alter()
are expensive, however, the performance of the page suffers massively.
The primary assumption that I am making here is that node_access_grants()
will never change within a single request for any given combination of $account and $op. That is what I would like confirmation on from others.
Locally, I am seeing node_access_grants()
reduced from ~1.2s to ~60ms. This particular site has some custom checks that are expensive, but the $grants
array is the same each time, so it appears there's no reason to call this multiple times.
Comment | File | Size | Author |
---|---|---|---|
#38 | drupal-statically_cache_node_access_grants-2199001-38.patch | 4.49 KB | Liam Morland |
Issue fork drupal-2199001
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 #1
erikwebb CreditAttribution: erikwebb commentedComment #3
moshe weitzman CreditAttribution: moshe weitzman commentedIf some implementation of this hook is expensive, that implementation should implement the cache. No sense in complicating core to defend against that, IMO.
Comment #4
erikwebb CreditAttribution: erikwebb commentedComment #5
erikwebb CreditAttribution: erikwebb commented@moshe weitzman - I definitely agree. The main reason why I'm submitting this to core is that it seems the basic behavior of calling this on every single node for a request seems fundamentally flawed. Regardless of performance, this function is doing unnecessary work that can be definitively stopped upstream from any custom implementations.
Comment #7
Wim LeersI don't know the node grants system at all, but it doesn't seem logical that the same grants would apply to all nodes within a given page load, i.e. that the same grants apply to all nodes, given any
$account
&$op
. It sounds like that is specific to your site?Having read https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function... & https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function... seems to confirm my hypothesis: you can have multiple realms, and the various nodes on a page may belong to different realms, in which case there would be different
$grants
?Comment #8
pwolanin CreditAttribution: pwolanin commented@Wim - I think you are looking at the wrong side of the API. This is finding the grants for the given account "Fetches an array of permission IDs granted to the given user ID.", and should be the same every time it's called with the same parameters.
I think this is on the right track - the result should be the same on every call.
In terms of the patch: needs work. In 7+ we generally don't use a $reset param (that's what drupal_static() handles).
Also, bugs to 8.x first, for actually getting fixed.
Comment #9
Wim LeersOk, glad to be wrong :)
Comment #10
erikwebb CreditAttribution: erikwebb commented@pwolanin - I'll upload a cleaned patch for D7 for reference, then reset to D8 to fix.
Comment #11
erikwebb CreditAttribution: erikwebb commentedComment #13
pwolanin CreditAttribution: pwolanin commentedLooks better
Comment #14
erikwebb CreditAttribution: erikwebb commentedThat looks good enough for me, since it's not being committed now anyhow. I'm re-assigning this to D8.
Comment #15
hefox CreditAttribution: hefox commentedThrew the d7 one onto an openatrium site and it cut down the time a bit
Comment #16
maximpodorov CreditAttribution: maximpodorov commentedThe patch is re-rolled for the current Drupal 7, and the important thing is added - static cache clearing upon node saving. If hook_node_insert() (or similar hook) implementations modify node grant records, this change will be invisible to node_access() without cache clearing.
Comment #17
mxr576I've just had the same idea and I'm glad to see that I'm not the only one who thinking about this type of solution to improve the performance of a Drupal site.
Without this patch a view on a site which lists only 8 nodes calls a hook_node_grants() and a hook_node_grants_alter() implementation 8+8 times, with this patch, they only called 1+1!
Why this patch is still in the queue since 2014? On sites with complex RBAC this core fix could cause a huge performance improvement. I started to testing this and I'll give you a feedback if I'll find any issue with the latest patch.
Comment #18
ndobromirov CreditAttribution: ndobromirov commentedAs the drupal_static has a default param, no need to duplicate the logic, just add a second param array().
I do not have environment at the moment to re-roll the patch. It will make it 4 lines shorter without any functional changes.
Comment #19
mxr576Nice catch, totally true.
Comment #20
AnybodyWell I think this issue is still very important. Has someone been using this in production for a longer period of time?
I could imagine out of memory issues for very large permission tables. Any problems or updates on this?
Comment #21
mxr576Using this on several production site with different hosting providers and PHP versions without noticing any problem.
Comment #22
AnybodyI finally solved my performance problems using this project: https://www.drupal.org/project/content_access_booster
It might be interesting for other users too, having performance problems because of access permissions.
Comment #23
Rob230 CreditAttribution: Rob230 as a volunteer and at CTI Digital commentedI think the way hook_node_grants() is called doesn't make any sense. It is passed a user, but is called per node. If there are 100 nodes on the page every module's implementation of hook_node_grants() will be called 100 times for the same user, and get the same result.
Surely it should be called once for the user? Or if not, then it should at least be statically cached as per this issue. The result doesn't vary per node, so why is it being called for every node?
As it is, I think I'll just have to statically cache my own implementation of hook_node_grants()... and probably apply additional caching beyond that, as the realms I have defined are unlikely to change unless the user is updated.
Comment #24
daniel.nitsche CreditAttribution: daniel.nitsche commentedExample of static caching hook_node_grants I've found in contrib: https://www.drupal.org/project/group/issues/3029849
Potentially facing a similar problem with the view_unpublished module: https://www.drupal.org/project/view_unpublished/issues/3097251
Comment #25
catchThis is still valid against 8.x/9.x and should still happen. 8.x code has not actually changed that much so should be a straightforward re-roll for someone.
Comment #26
apadernoComment #27
Neslee Canil PintoRerolled the patch, please review
Comment #29
BR0kENRe-roll for 9.1.x.
$cache[$uid][$op]
to$cache[$op][$uid]
because my tests shown better memory use with this structure.The test I used:
$account->id()
, which can be done just once instead of repeatedly executing the function that obviously returns the same result.Comment #31
BR0kEN#29 with adjusted tests.
Comment #32
BR0kEN#31 with a reduced code repetition.
Comment #34
BR0kENRe-roll for 9.3.x.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This could use an issue summary update using the default template for steps to reproduce, proposed solution, and remaining tasks. To help the committers later.
Comment #38
Liam MorlandRe-roll
Comment #39
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #41
Liam MorlandComment #42
AnybodyThanks for the reroll @Liam Morland! Code LGTM! Could you perhaps provide it as MR so the tests can run and we can hopefully finish this soon?
Still has a "Needs issue summary update" tag.
Comment #43
Liam MorlandIt is a MR; see #40.
Comment #44
AnybodySorry @Liam Morland shame on me, I didn't see #40 just #38 -.- Thank you!
Comment #45
smustgrave CreditAttribution: smustgrave at Mobomo commentedHave not tested but see it was previously tagged for issue summary which still appears to be needed. Recommend using standard issue template even if the section doesn't apply, leaving N/A helps reviewers more then it missing.