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

Unuse ActiveRecord::Base#cache_key #8185

Merged
merged 4 commits into from Aug 19, 2018
Merged

Conversation

abcang
Copy link
Contributor

@abcang abcang commented Aug 13, 2018

Change to give the instance key instead of cache_key to the key of the cache. Rails automatically processes the version information by specifying the instance as a key.

@@ -42,7 +42,7 @@ def show
format.json do
skip_session!

render_cached_json(['activitypub', 'actor', @account.cache_key], content_type: 'application/activity+json') do
render_cached_json(['activitypub', 'actor', @account], content_type: 'application/activity+json') do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an array is given to the key of Rails.cache.fetch, the key will be like activitypub/actor/accounts/1.

@@ -17,7 +17,7 @@ class Api::V1::StatusesController < Api::BaseController
CONTEXT_LIMIT = 4_096

def show
cached = Rails.cache.read(@status.cache_key)
cached = Rails.cache.read(@status)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not related to this PR, but the cached account information may be old ...

@Gargron
Copy link
Member

Gargron commented Aug 15, 2018

Could you explain the significance of these changes? I do not understand

@abcang
Copy link
Contributor Author

abcang commented Aug 16, 2018

When cache_versioning is true from Rails 5.2 cache_key no longer includes the value of updated_at, and version information such as the value of updated_at is now managed by ActiveSupport::Cache::Store. There is an advantage that the number of keys increases for each version, and wasteful caching is not created.
Version information can be given by using the version option of methods such as Rails.cache.write, but if an instance of ActiveRecord is given to the key of cache, the value of updated_at is automatically used as version.
ActiveRecord of Rails 5.2 cache_versioning is true by default. By setting cache_versioning to false it is possible to include the value of updated_at in cache_key as before, but it is better to keep cache_versioning to be true for cache efficiency.

reference: rails/rails#29092

@Gargron
Copy link
Member

Gargron commented Aug 18, 2018

@abcang How does this PR interact with the statuses/statuses_stats separation? Currently we fetch updated_at from statuses_stats if it's more recent. But if we won't use updated_at, then updates on StatusStat need to bump the Status' cache version

@abcang
Copy link
Contributor Author

abcang commented Aug 19, 2018

When cache_collection is used, since updated_at of statuses_stats is read by Status.cache_ids, it works correctly. I think that it will not work properly if Status.cache_ids is not used. Also, since json which is used for ActivityPub does not use information of statuses_stats, it seems no problem.

@Gargron Gargron merged commit 9e75aa3 into mastodon:master Aug 19, 2018
@abcang abcang deleted the unuse_cache_key branch August 19, 2018 14:50
kyori19 pushed a commit to kyori19/mastodon that referenced this pull request Sep 20, 2018
* Unuse ActiveRecord::Base#cache_key

* Enable cache_versioning

* Call cache_collection
Gargron added a commit that referenced this pull request Oct 27, 2018
Fix #8711

Media attachments are generally immutable, but admins can update
the sensitive flag, and this would ensure the change is visible
instantly. Same for updates to status stats. That is a regression
from #8185, because even the correct updated_at fetched from a join
doesn't seem to invalidate the cache.
Gargron added a commit that referenced this pull request Oct 28, 2018
* Reset status cache when status_stat or media_attachment updates

Fix #8711

Media attachments are generally immutable, but admins can update
the sensitive flag, and this would ensure the change is visible
instantly. Same for updates to status stats. That is a regression
from #8185, because even the correct updated_at fetched from a join
doesn't seem to invalidate the cache.

* Remove join from Status#cache_ids since it has no effect
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

2 participants