Posted by miro_dietiker on December 9, 2008 at 10:59am
5 followers
| Project: | ACL |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
for hook_node_grants:
In acl_node_grants you fetch a acl_id list by sql each time an operation is requested. Why not caching this result per ($account, $op) ?
since core node.module @2122 function node_access_grants doesn't cache values it could make sense to cache that..?
Although i didn't benchmark this and i was just working on understanding the code while i'd expect some potential of this.
Comments
#1
Do you have any indication that this might be called more than once (not counting calls from devel_node_access()) with the same ($account, $op)?
If caching were beneficial, it should be implemented in node_access_grants(), so that all NA modules can benefit.
I don't know this, but from the fact that node_access_grants() doesn't implement caching I infer that caching isn't worthwhile here, until we have evidence to the contrary.
#2
Completely untested: I was thinking about providing multiple (block?) views on a page.
E.G. if i list a forum as a page view and then add another block with most recent overall topics/posts and others...
Wouldn't this result in two view operations as invoke_all regarding hook_node_grants?
I'll report back when my project clearly shows that state.
#3
Yes, that's a scenario that could result in two node_access_grants('view') (and ultimately two acl_node_grants()) calls. How common is this? What benefit can we expect from caching (for ACL it's a simple query)? What overhead?
Either way I still maintain that if caching would be beneficial overall, it should be done in node_access_grants(), not in each module. Feel free to take this to the core queue, preferably with a node_access_grants() patch and some comparative measurements for call once and call twice.
#4
acl_node_grants() can be called not only from node_access_grants(). On my project I have 28 calls acl_node_grants(). Half from node_access_grants(), half from grants_by_module() (module_grants module).
I think is better to cache query results. Its just a few lines of code.
Attached patch for 6.x-1.x version. Please, review it.
#5
But how many of these 28 would actually be cache hits, i.e. same $account and same $op?
I'm still unconvinced that it makes sense for every node access module to cache its own results. If such caching is needed, then it should be done by the caller.
You don't have devel_node_access enabled, do you?
#6
Hans, thanks for your opinion, I think so too.
There's just one strong reason why it should be done in modules — because Drupal 6 core is frozen and we can not patch it anymore. I think you know how far Gabor sends those people who suggest something to improve radical in core :)
ACL is most popular access module, so if it has caching, that means that it serves most of the users.
#7
We know just about nothing about the usage patterns we're trying to address here. Why 14 + 14? Is Leksat's page evaluating grants for 14 different users? Then we would increase ACL's memory consumption by a factor of 14!
If we're really evaluating the same query 14 times, then why does this happen? Should ACL try to make up for dumb callers?
ACL is critical infrastructure. Above all it needs to be reliable and predictable. If we want to change something, we have to avoid causing unexpected problems.
This issue has been dormant for two years. There has been plenty of time to investigate caching for D7. In which issue was this done?