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

perf(token_is_current?): add simplistic cache to reduce overhead of redundant token checks during validation calls #272

Merged
merged 1 commit into from
Jun 18, 2015

Conversation

booleanbetrayal
Copy link
Collaborator

Within DeviseTokenAuth::Concerns::User#token_is_valid? we're seeing the BCrypt::Password.new(token_hash) + string comparison (casting) operation itself take approximately 75ms on average when validating tokens, which makes it hard / impossible to reach our goal of < 200ms response times for request handling.

This PR adds a very simplistic caching mechanism that will lookup the check value, saving unnecessary overhead on redundant token / hash checks. It can be iterated on further if a more robust caching implementation is needed / desired.

This will primarily benefit environments where change_headers_on_each_request is disabled, but will still help requests that occur within the batch_request_buffer_throttle threshold.

Thanks to @nbrustein for the implementation.

…edundant token checks during validation calls
@booleanbetrayal booleanbetrayal force-pushed the improve_token_is_current_perf branch from 425594e to 27cad66 Compare June 17, 2015 18:21
@nicolas-besnard
Copy link
Contributor

What about the memory usage ?

@booleanbetrayal
Copy link
Collaborator Author

should be negligible @nicolas-besnard ... the cache resets at 1000 entries and each entry uses < 100 bytes (83 fixed-length for the key) + boolean value. So ... less < 100k for the full collection.

@booleanbetrayal
Copy link
Collaborator Author

FWIW - we saw about this reduce overhead from an additional ~75ms -> ~5ms (once cached) per request

@nicolas-besnard
Copy link
Contributor

Ok, you can do what ever you want, I don't care anymore about my memory :D

Nice PR ;)

@booleanbetrayal
Copy link
Collaborator Author

Gonna run this a little longer in prod before merge, but so far, it's saving A LOT of time on our requests (we have token rotation disabled, FWIW).

@booleanbetrayal
Copy link
Collaborator Author

Can't find any downside to this and it's speeding everything along beautifully. Merging!

booleanbetrayal added a commit that referenced this pull request Jun 18, 2015
…_perf

perf(token_is_current?): add simplistic cache to reduce overhead of redundant token checks during validation calls
@booleanbetrayal booleanbetrayal merged commit 3ed1b91 into master Jun 18, 2015
@lynndylanhurley
Copy link
Owner

Fantastic work, thanks @booleanbetrayal!!!

@booleanbetrayal
Copy link
Collaborator Author

Thanks to @nbrustein =]

@irobayna
Copy link

👍

@booleanbetrayal booleanbetrayal deleted the improve_token_is_current_perf branch June 19, 2015 14:58
@nicolas-besnard
Copy link
Contributor

When are you bumping this gem to a new version ? I'd love to test this feature in production ;)

@booleanbetrayal
Copy link
Collaborator Author

@nicolas-besnard - Just bumped and pushed 0.1.32.beta10 to RubyGems. Let me know if you have any issues!

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.

5 participants