Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Acl::SetRule performance is very slow #4

Open
steverhoades opened this issue Oct 3, 2015 · 7 comments
Open

Acl::SetRule performance is very slow #4

steverhoades opened this issue Oct 3, 2015 · 7 comments

Comments

@steverhoades
Copy link

We are dealing with rules in the 70k - 100k 1k - 2k range. It currently takes setRule around 2.5 seconds to process all of these rules.

I have been able to increase the performance of this method by roughly 40% by simply eliminating the use of array_merge.

Suggested Change Acl line:580 to:

foreach($children as $child) {
    $resources[] = $child;
}

Suggestions on how this method can be improved further would be great appreciated. The tree that is being generated takes approximately 100-130ms to serialize/unserialize.

Note: Acl configuration will be cached in Production.

EDIT:
Sorry rules are in the 1500 - 2000 range. Roughly 300 roles and 300 resources.

@steverhoades
Copy link
Author

Here is the current benchmarks running 1538 rules.

Function                                        Count   Incl.       Excl.   Average (ms) 
Zend\Permissions\Acl\Acl::setRule()             1538    1,463.07    832.59  0.95    
Zend\Permissions\Acl\Acl::getRules()            70450   319.49      292.49  0.00    
Zend\Permissions\Acl\Acl::getResource()         70214   132.21      98.47   0.00    
Zend\Permissions\Acl\Acl::getChildResources()   70210   124.85      111.78  0.00    
Zend\Permissions\Acl\Acl::hasResource()         70808   34.23       34.17   0.00    

@Ocramius
Copy link
Member

Ocramius commented Oct 4, 2015

@steverhoades you can try patching the line you suggested, but you'd need to verify edge cases with string keys and array keys first.

@steverhoades
Copy link
Author

@Ocramius good point.
How about:

foreach($children as $key => $child) {
    $resources[$key] = $child;
}

@Ocramius
Copy link
Member

Ocramius commented Oct 4, 2015

@steverhoades can work, yes.

@Ocramius
Copy link
Member

Ocramius commented Oct 4, 2015

@steverhoades again, I'd start from the tests: the snippet above overwrites numeric keys.

@steverhoades
Copy link
Author

@Ocramius I should know better, I'll check out the tests. From reading the code it appears as though the keys should be resourceIds... perhaps i have that wrong.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-permissions-acl; a new issue has been opened at laminas/laminas-permissions-acl#6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants