Skip to content
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

Save permissions structure to Rails.cache if a cache key is passed #21

Merged
merged 3 commits into from
Aug 31, 2022

Conversation

dansingerman
Copy link
Contributor

With a large roles.yml, even if you are using a UserPermissionCachingDecorator to intercept db access (or using #20), the calls to permit can be pretty slow.

In an app with ~200 entries in the roles.yml it was taking ~130ms to process all the permit calls.

This change allows a cache_key parameter to be passed in, and if it is, the structure that is passed to CanCanCan is cached by rails, saving most of the processing time.

Note this should not be a replacement for #20 - which is caching at a different level and should also be implemented.

@andrewculver andrewculver merged commit dcc6eae into main Aug 31, 2022
@andrewculver andrewculver deleted the cache_permit_calls_to_rails_cache branch August 31, 2022 16:00
@adampal
Copy link
Contributor

adampal commented Sep 14, 2022

@dansingerman sorry I'm a bit late to the party on this one.
The change looks good. I just wanted to ask your thoughts on using OpenStruct rather than a Hash. Given this is a performance optimisation, a Hash should be much faster than an OpenStruct. The ruby lang guide (here - https://docs.ruby-lang.org/en/master/OpenStruct.html) has this:

Creating an open struct from a small Hash and accessing a few of the entries can be 200 times slower than accessing the hash directly.

Given this is just internal, my thinking is we'd be better off just using a Hash and missing out on the syntactic sugar provided by OpenStruct in favour of performance.

@dansingerman
Copy link
Contributor Author

@adampal I benchmarked OpenStructs vs Hashes and you're totally right - so I've raised a PR here: #25

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

Successfully merging this pull request may close these issues.

None yet

3 participants