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

collection cache versioning #34378

Closed

Conversation

@lsylvester
Copy link
Contributor

@lsylvester lsylvester commented Nov 5, 2018

Cache versioning enables the same cache key to be reused when the object being cached changes by moving the volatile part of the cache key out of the cache key and into a version that is embedded in the cache entry.

This is already occurring when the object being cached is an ActiveRecord::Base, but when caching an ActiveRecord::Relation we are currently still putting the volatile information (max updated at and count) as part of the cache key.

This PR moves the volatile part of the relations cache_key into the cache_version to support recycling cache keys for ActiveRecord::Relations.

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 5, 2018

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@gingerlime
Copy link
Contributor

@gingerlime gingerlime commented Nov 17, 2018

👍 for this feature. I accidentally created a similar PR. Just curious, but why define collection_cache_versioning instead of relying on a single cache_versioning config? I guess it's more configurable, but I would imagine that if you want cache versioning, then you want it across the board.

@lsylvester
Copy link
Contributor Author

@lsylvester lsylvester commented Nov 19, 2018

@gingerlime From what I understand - cache_versioning = true is going to become the default in Rails 6. Having these behind a feature config that is enable by default would seem to defeat the point of the feature config.

If people have overwritten collection_cache_key, they will need to change it to collection_cache_version to enable the recycling of cache keys - and this will give them a change to do so while there app is running in Rails 6.

If cache_versioning was unreleased at this point then I would agree that they should be behind the same flag.

@gingerlime
Copy link
Contributor

@gingerlime gingerlime commented Nov 19, 2018

Thanks @lsylvester. I see it as an omission from the original feature, but maybe it was intended this way? I'm not sure.

Besides that, this PR should hopefully resolve (what looks to me like) a performance regression bug that happens in some cases even if you don't switch cache_versioning on (and already with Rails 5.2).

In any case, keen to see this merged one way or another :)

@lsylvester
Copy link
Contributor Author

@lsylvester lsylvester commented Nov 20, 2018

@gingerlime I wasn't ware of that performance regression. That is clearly a bug, and while I don't think that we can enable full relation cache versioning without some time to update collection_cache_key methods - I think we can avoid this performance regression.

In my opinion - if an object response to cache_key but not cache_version then its cache_version should be nil even it it response to to_a.

Let me know if you want to have a go at that otherwise I will make a PR when I have time.

@gingerlime
Copy link
Contributor

@gingerlime gingerlime commented Nov 21, 2018

Thanks @lsylvester. I didn't get any response regarding this, so it's good that I'm not the only one who thinks this is a bug :)

I guess the simplest fix would be to implement cache_version (and return nil) on the AR relation. In theory, if someone relied on the relation expanding (using to_a) and then each element returning their own cache_version, then this can break this assumption though... But I somehow doubt there's any legitimate use case for this. I'll try to find some time to create another PR for this. No clue about the timelines for bug fixes and features like this one though, but hope someone can push this forward :)

EDIT: I guess this simplest fix would conflict with your PR however... So another option is to implement this check inside the normalize_version.

gingerlime pushed a commit to gingerlime/rails that referenced this issue Nov 21, 2018
* After introducing cache versioning, even with cache versioning off
  there's a performance regression when passing an Active Record
  relation to cache
* This happens in ActiveSupport::Cache inside `normalize_version`
* This method would check if the relation responds to cache_version
  and if not, would recrusively normalize it with `to_a`
* This would lead to the relation being retrieved from database and
  enumerated, causing the performance regression
* This fix simply adds `cache_version` returning `nil` to Active Record
  relations
* This is a temporary stopgap, until relation cache versioning is
  implemented. See rails#34378
@gingerlime
Copy link
Contributor

@gingerlime gingerlime commented Nov 22, 2018

@rafaelfranca referencing my comment

This bug can cause quite severe performance regression from my experience, and it largely goes unnoticed, unless you actively measure performance. Hope that #34378 gets merged quickly and also backported, to fix existing versions of rails affected by this regression.

Is there any timeline for moving this forward? Happy to help if I can.

@lsylvester lsylvester force-pushed the collection-cache-versioning branch 2 times, most recently from 3f32b21 to 126fa44 Jan 30, 2019
@lsylvester lsylvester force-pushed the collection-cache-versioning branch from 126fa44 to 45e650f Jan 30, 2019
@koukou73gr
Copy link

@koukou73gr koukou73gr commented Apr 4, 2019

Will this make it into Rails 6 ?

@eileencodes eileencodes added this to the 6.0.0 milestone Apr 4, 2019
@kaspth
Copy link
Member

@kaspth kaspth commented Apr 16, 2019

Hey @lsylvester, sorry for never getting to review this! After a pretty huge refactoring in 10919bf I've just gone ahead and merged this in 1da9a7e. I opted not to add the collection_cache_version override because the collection_cache_key in integration.rb was marked as nodoc, so I don't think we need that feature. cc @kamipo

Thanks for working on this, I'm looking forward to this in 6.0! ❤️

@kaspth kaspth closed this Apr 16, 2019
kaspth added a commit that referenced this issue Apr 16, 2019
Signed-off-by: Kasper Timm Hansen <kaspth@gmail.com>
@aleksandrs-ledovskis
Copy link

@aleksandrs-ledovskis aleksandrs-ledovskis commented May 29, 2019

@kaspth Any chance of this being backported into v5.2.x? I would reason that collection caching being a fully working feature in v5.1 and regressing in v5.2 makes this a worthy candidate.

@kaspth
Copy link
Member

@kaspth kaspth commented May 29, 2019

It’s not broken in 5.2, collections just weren’t supported. They will be in 6.0.

@aleksandrs-ledovskis
Copy link

@aleksandrs-ledovskis aleksandrs-ledovskis commented May 29, 2019

@kaspth

class FooController < ApplicationController
  def bar
    @xyz = Xyz.all
  end
end
<% cache(@xyz) do %>
  <!-- -->
<% end %>

In 5.1.x the code as outlined above does cache collection and on non-initial hits only queries for cache_key.
In 5.2.x the caching also works, but causes extra to_a unwrapping of relation due mandatory #normalize_version call.

Would you like me building a sample test case to demonstrate further?

@kaspth
Copy link
Member

@kaspth kaspth commented May 29, 2019

Gotcha, feel free to send a fix then, don’t think we want to back port since it’s a lot of code.

@aleksandrs-ledovskis
Copy link

@aleksandrs-ledovskis aleksandrs-ledovskis commented May 29, 2019

@kaspth Just to be completely sure on the meaning of "fix" & spare wasted time - are you proposing I create a backport of this PR to 5-2-stable? Because #34503 already proposed "a fix" that wasn't OK'ed by @rafaelfranca.

@feliperaul
Copy link

@feliperaul feliperaul commented Jul 22, 2019

@kaspth Kasper, I'm using collection.cache_key in 5.2 inside Rails.cache.fetch calls, where I'm expected to manually build my cache keys for a collection.

When cache_key evolved to cache versioning for individual records, returning a stable key, a new method cache_key_with_version was provided, that returned the old volatile string with timestamp. So it was a 'breaking change', but at least we had a way to update our code to avoid breakage.

However, after this PR was merged in 1da9a7e, cache_key changed behavior and there's no cache_key_with_version to fallback on, meaning not only it's a breaking change (cache_key is a public method), but also we don't have any way to update our code to avoid breakage inside Rails.cache.fetch calls, where we have to build the key ourselves.

  • Shouldn't we have collection.cache_key_with_version to keep consistency with the record instance methods? (that provide both cache_key and cache_key_with_version?
  • If not, what should I use inside a Rails.cache.fetch where I'm building a key manually for a collection?

@feliperaul
Copy link

@feliperaul feliperaul commented Jul 22, 2019

Pinging @lsylvester as well

@bdmac
Copy link
Contributor

@bdmac bdmac commented Mar 5, 2020

Has anything happend with @feliperaul's comment above about relations not actually having a cache_key_with_version method? It seems strange that turning on versioning makes it so cache_key no longer includes the version but then not have the cache_key_with_version method on relations to fall back on.

It also seems like this would not "just work" with Rails.cache.fetch(association) since it looks like the code here tries cache_key_with_version which associations do not have and then drops down to cache_key which no longer includes a version if you turn on versioning.

@bbugh
Copy link

@bbugh bbugh commented Feb 12, 2021

If anyone else ends up here from a web search looking into why cache_key_with_version isn't on collections, it was added in 6.1 from this PR: #38119

If you have an Rails version >= 6.0 and < 6.1, you can put this backport in an initializer:

if Rails.version < "6.1.0"
  class ActiveRecord::Relation
    def cache_key_with_version
      if version = cache_version
        "#{cache_key}-#{version}"
      else
        cache_key
      end
    end
  end
else
  raise "ActiveRecord::Relation cache_key_with_version backport no longer required in Rails >= 6.1.0."
end

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

Successfully merging this pull request may close these issues.

None yet

10 participants