Remove code duplication in ActiveSupport::Cache #31065
Conversation
|
(@rails-bot has picked a reviewer for you, use r? to override) |
bogdan
force-pushed
the
cleanup-cache-key
branch
from
3dae7ae
to
1967f2e
Compare
| @@ -57,7 +57,7 @@ def view_cache_dependencies | |||
| # Convenience accessor. | |||
| def cache(key, options = {}, &block) # :doc: | |||
| if cache_configured? | |||
| cache_store.fetch(ActiveSupport::Cache.expand_cache_key(key, :controller), options, &block) | |||
| cache_store.fetch([:controller, key], options, &block) | |||
There was a problem hiding this comment.
This sounds like a behavioural change
There was a problem hiding this comment.
Nope: in previous implementation controller as namespace prepends the key just in the same way as if the key has two parts in current implementation.
|
|
||
| expanded_cache_key << retrieve_cache_key(key) | ||
| expanded_cache_key | ||
| [namespace, ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"], retrieve_cache_key(key)].compact.join("/") |
There was a problem hiding this comment.
This is more allocations in a reasonably hot path, and also seems less readable to me
There was a problem hiding this comment.
This is less allocation: in previous impelementation you allocate many strings. In this implementation you allocate one array and the dup of it (via compact).
We can change compact to tap(&:compact!) if you care about allocations.
Put 👎 and I will revert to previous implementation. As I am not super concerned.
I am not sure what is "a reasonably hot path". Can you explain?
There was a problem hiding this comment.
Previously it was one string in expanded_cache_key (two if namespace), plus one more if prefix. With this change it's the array literal, the compacted array, and the joined string.
"hot path" = code-path that's called very frequently, deserving of performance concern; reasonably (in this context) = not very, but somewhat [hot].
| # object responds to +cache_key+. Otherwise, +to_param+ method will be | ||
| # called. If the key is a Hash, then keys will be sorted alphabetically. | ||
| def expanded_key(key) | ||
| return key.cache_key.to_s if key.respond_to?(:cache_key) |
There was a problem hiding this comment.
Is there relevant history to why this method wasn't taught about cache_key_with_version?
There was a problem hiding this comment.
cache_key_with_version was added by @dhh not so long ago. I was in discussion on that feature and we didn't figure out this code dup at that moment. It has no 100% test coverage so far.
bogdan
force-pushed
the
cleanup-cache-key
branch
from
1967f2e
to
57f0e3d
Compare
Summary
Remove code duplication in the implementation of the cache key