search

zendframework / zend-permissions-acl

BSD 3-Clause "New" or "Revised" License
60 stars 23 forks source link

Acl::SetRule performance is very slow #4

Open steverhoades opened 8 years ago

steverhoades commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@Ocramius good point.
How about:

foreach($children as $key => $child) {
    $resources[$key] = $child;
}
Ocramius commented 8 years ago

@steverhoades can work, yes.

Ocramius commented 8 years ago

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

steverhoades commented 8 years ago

@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 commented 4 years ago

This repository has been closed and moved to laminas/laminas-permissions-acl; a new issue has been opened at https://github.com/laminas/laminas-permissions-acl/issues/6.