-
Notifications
You must be signed in to change notification settings - Fork 335
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
Avoid RedisCacheStore#increment on Rails 6+ #597
Conversation
Let me know if there's interest in merging this & I'll rebase. |
I'd really like to see it land. In some brief local application-level benchmarking, the patch is quite a bit faster. In simpler applications or situations where Redis latency is worse, I suspect the impact would be even larger. Using derailed_benchmarks:
|
Yes, please. Thank you. |
e27e9e7
to
edf0a82
Compare
It looks like there's test failures with redis-activesupport against ActiveSupport 7.1.1 - it tries to use I've fixed it by adding the requires to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
Thank you!
Pending conflicts with latest changes in main.
I'm away for a few weeks, but can pick this up again in November. |
Hi, I think this is a nice improvement 👏 I was checking the PR and noticed that there are no tests for ActiveSupport < 6, so went ahead and added: # gemfiles/active_support_5_redis_cache_store.gemfile
source "https://rubygems.org"
gem "activesupport", "~> 5.2.0"
gem "redis", "~> 5.0"
gemspec path: "../" When running the tests EDIT: |
I'm very interested in seeing this merged, is there anything I can do to help move it forward? |
Hi @mitchellhenke, I think we are waiting for @jdelStrother to fix the CI and that should be it 👌 He said he was out til November, so he'll probably be coming back soon. |
Yep, let me try & take a look at it today |
Rails has handled increment+expires_in since 6.0 (rails/rails@9d5b02e), and more recent versions add pipeline optimizations on top of that.
f436d0a
to
ef6fb5e
Compare
Rebased and fixed the |
Thanks for your contribution @jdelStrother 🙌 |
As far as I can tell, Rails has handled increment+expires_in since
6.0 (rails/rails@9d5b02e), and more recent versions add pipeline optimizations on top of that.
How about avoiding our own custom RedisCacheStoreProxy#increment method on modern versions of ActiveSupport?
This also avoids an extra GET (from the
if options[:expires_in] && !read(name)
call).