Skip to content

[Security] Make AccessDecisionManager much faster #49670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Mar 11, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

When you are using a lot of voters, you need better caching. With inspiration from #43066 by @jderusse I managed to squeeze some more performance from the AccessDecisionManager.

I do about 7000 calls to AccessDecisionManager::getVoters(). It takes me 369ms.
With this PR, it takes about 30ms.

The trick is that I don't loop over all Voters all the time. If I seen the object and attribute before then I just returns the voters I previously calculated.


Performance:

When do benchmark on an application with 40 voters I got:

Metric From To
Wall time 1150ms 859 ms
I/O Wait 135 ms 154 ms
CPU 1020ms 705 ms
Memory 11.8 MB 12.9 MB

I would love a few more set of eyes on this PR.

@Nyholm
Copy link
Member Author

Nyholm commented Mar 11, 2023

I spent 3 hours last night considering if I should make this PR or not.
I had this feeling that something wasn't right.. This fix was "too simple" for @jderusse or anyone else to miss.

I slept on it but still couldn't understand the why I got so massive performance gain... It turns out that I had xdebug on while debugging 🤦🏽

When I re-ran the profiles it turns out they are pretty much the same. I dont think this complexity is worth the almost non existent gain.

@Nyholm Nyholm closed this Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants