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

Cache Support at AMS 0.10.0 #693

Merged
merged 1 commit into from
Feb 4, 2015
Merged

Conversation

joaomdmoura
Copy link
Member

This is a implementation of cache support to the new version of AMS.

It started originally at the mailing list:
AMS: Caching with Matthijs Langenberg.
New Cache Support on AMS. with me

This implementation proposes x different approaches of the old versions of AMS.

  • A different syntax. (no longer need the cache_key method)
  • An options argument that have the same arguments of ActiveSupport::Cache::Store, plus a key option that will be the prefix of the object cache on a pattern "#{key}-#{object.id}".
  • It cache the objects individually and not the whole Serializer return, re-using it in different requests (as a show and a index method for example.)

There are many benefits on this approach:

  • You can easily know which will be your final cache key, so you can expire it with any custom code.
  • You can use all options of ActiveSupport::Cache::Store (:expires_in for example)
  • All objects will be cached individually and re-used on any other return needed
  • It keeps you Serializer clean by not depending on old cache_key method.

@hhff
Copy link

hhff commented Oct 23, 2014

👍 👍 👍

@zlx
Copy link

zlx commented Oct 24, 2014

👍

@zlx
Copy link

zlx commented Oct 24, 2014

It's awesome to bring cache back, Thank you.

I read your idea in New Cache Support on AMS, and found it resolve the problem to reuse cache between index and show actions.

I have use AMS on some projects and find two truth:

  1. we always have AMS object with many associated objects
  2. the AMS entire cache is often expired since associated object expired

In fact, I have try to design a new cache strategy, and benchmark result always >50x faster than no cache.

here is my demo app for benchmark and here is my Cache Strategy base AMS 0.9

I hope it's useful for you to design the new cache strategy for AMS 0.10

@joaomdmoura
Copy link
Member Author

Hey @zlx, I have just checked your cache suggestion at 0.9.0. Btw, thank you for contributing!
I was also missing cache, it couldn't be outside 0.10.0.

Yes, this implementation focus on make the cached objects as reusable as possible, it cache the serializer output, each major object individually, and does not expire it unless you tell it to. (Using expires_in option or with any custom code you want). It seem to fit to the major needs and is pretty straightforward.

I'm always hungry for new ideas and suggestions so let's keep improving it even after merged 😄

@zlx
Copy link

zlx commented Oct 30, 2014

Hi, @joaomdmoura I find in real world, sometimes in one AMS object, we want to not cache some attributes, eg: is_follow like twitter, so we can make cache live more long time.

consider add this support? thus the we can just use AMS for cache API.

@joaomdmoura
Copy link
Member Author

Hi @zlx yeah, totally agree. I'll work on it :)
Thank you for the suggestion 👍

@zlx
Copy link

zlx commented Oct 30, 2014

Awesome. look forward your great job

on a pattern ```"#{key}-#{object.id}"```.

**[NOTE] Every object is individually cached.**
**[NOTE] The cache isn't automaticly expired after update an object.**
Copy link
Member

Choose a reason for hiding this comment

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

Typo, automaticly.

@kurko
Copy link
Member

kurko commented Nov 12, 2014

@joaomdmoura can you rebase?

Summoning @steveklabnik. Is this something you want for AMS right now?

@steveklabnik
Copy link
Contributor

In general, caching is something we want in AMS, yes 😄

@joaomdmoura
Copy link
Member Author

@kurko Of course, I might add some more commit too. I'll do this tonight 😄

@BenMorganIO
Copy link

Hey, has this been actively worked on lately? If caching is required for some apps, would 0.8 be a better fit?


```ruby
class PostSerializer < ActiveModel::Serializer
cache key: 'post', expires_in: 3.hours
Copy link
Member

Choose a reason for hiding this comment

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

@joaomdmoura could we make key optional? Then we could use object.cache_key by default. Makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kurko I'm not sure if I have already done it 😕 But I'll check it.

@kurko
Copy link
Member

kurko commented Jan 21, 2015

Could you rebase this? We want to move forward with caching 😁

@joaomdmoura
Copy link
Member Author

haha Actually I already started it 3 times 😞 but the tests always end up broken.
I'll try to really do this asap. 😄

@kurko
Copy link
Member

kurko commented Jan 22, 2015

I'm so sorry :( Getting this in is my number 1 priority now, I promise.

Could you also squash your commits into one with a nice title and description?

@joaomdmoura
Copy link
Member Author

@kurko It's done! Rebased, working and squashed. 😄

klass = serializer.class
if klass._cache
_cache_key = klass._cache_key || serializer.object.class.name.downcase
klass._cache.fetch("#{_cache_key}-#{serializer.object.id}", klass._cache_options) do
Copy link
Member

Choose a reason for hiding this comment

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

I think I wasn't clear in my previous comment (https://github.com/rails-api/active_model_serializers/pull/693/files#diff-04c6e90faac2675aa89e2176d2eec7d8R252). Basically, I believe we should use model.cache_key here when key is not used (docs in http://api.rubyonrails.org/classes/ActiveRecord/Integration.html). If you don't specify a key, it'd use object.cache_key:

Person.find(5).cache_key  # => "people/5-20071224150000"

The problem with using only an id here is that if we update the model, it'll reuse an old cache because update_at is disregarded. With cache_key, it'll regard update_at. With a custom key, I believe updated_at should also be included in this identifier.

What do you think?

Choose a reason for hiding this comment

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

👍

@joaomdmoura
Copy link
Member Author

@kurko totally agree. I'll work on this.

@kurko kurko mentioned this pull request Jan 26, 2015
@joaomdmoura
Copy link
Member Author

@kurko there is a down side on adding the updated_at to the key that I would like to discuss.
One of the reason I didn't added this before was:

On the later versions of cache on AMS there wasn't a way to expire an specific cache on an abstracted logic, because there was no way to know the cache key.
I added the key option and defined this simple "key-id" pattern in order to make it simple to expire this cache on any part of your application, like a background job for example.

Do you think there is no need to worry about it? Or do you see some workaround?

@kurko
Copy link
Member

kurko commented Jan 28, 2015

@joaomdmoura

  1. when using updated_at, changed objects would automatically make caches obsolete. I believe this is what people would expect to be the convention.
  2. expiring caches wouldn't be that hard. I'm pretty sure you could find objects with a Glob and wildcard.
  3. people can still disregard this and set their own keys. Perhaps another option would be sense if you feel uncomfortable, e.g cache key: "post", identifier: :id vs cache key: "post", identifier: [:id, :updated_at]

@joaomdmoura
Copy link
Member Author

@kurko yeah, u're probably right.
I may not be taking into account the most expected behaviour.
I'll move forward on this feature so that we can finally merge it.

@@ -1,3 +1,4 @@
require 'pry'

Choose a reason for hiding this comment

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

A wild pry appears

Choose a reason for hiding this comment

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

Observe, pry in its natural habitat.

Copy link
Member Author

Choose a reason for hiding this comment

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

hahahahahah sorry people, already removed it. 😄

@nwjsmith
Copy link

Is there a way for this cache-ing scheme to support something like Fragment/Russian Doll Caching? If not, this leaves AMS at a serious disadvantage to JBuilder for higher-performance apps.

One of the things that is nice about the old def cache_key-based caching scheme was that the key could be generated dynamically. You can, for example, generate a key based on the cache key for associated objects, or the current Git commit. Will there still be hooks in this new caching scheme to allow for that?

@sandstrom
Copy link

Awesome! Smooth ⛵

@BenMorganIO
Copy link

YAY

@kenips
Copy link
Contributor

kenips commented Feb 6, 2015

❤️ ❤️ ❤️

@kurko can we get a 0.10.0.alpha released soon?

@kurko
Copy link
Member

kurko commented Feb 6, 2015

@kenips we need to get fragment cache first :) Check #802.

@joaomdmoura
Copy link
Member Author

@kenips It's already WIP. I'll try to get it done next week.
The first version is already working but there is some optimisations that I need to work on, and also think how I will deal with relationships.

joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 11, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 12, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 12, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 12, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 12, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 20, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 20, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 20, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Feb 21, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 2, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 2, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 2, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 10, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 23, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 23, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 23, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 24, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 27, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Mar 27, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Apr 2, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
joaomdmoura added a commit to joaomdmoura/active_model_serializers that referenced this pull request Apr 5, 2015
It's an upgrade based on the new Cache implementation rails-api#693.
It allows to use the Rails conventions to cache
specific attributes or associations.
It's based on the Cache Composition implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants