-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding Fragment Cache to AMS #810
Conversation
@kurko and it's alive ;) made with ❤️ |
@@ -294,6 +295,25 @@ On this example every ```Post``` object will be cached with | |||
the key ```"post/#{post.id}-#{post.updated_at}"```. You can use this key to expire it as you want, | |||
but in this case it will be automatically expired after 3 hours. | |||
|
|||
### Fragmenting Caching | |||
|
|||
If there is some API endpoint that shouldn't be fully cached, you can still optmize it, using Fragment Cache on the attributes and relationships that you want to cache. |
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.
Typo, optime
.
/cc @spastorino |
7df73c0
to
3de7324
Compare
@kurko, Updated, thank you for the review! Just let me know if you find anything else. |
module ActiveModel | ||
class Serializer | ||
class Adapter | ||
extend ActiveSupport::Autoload | ||
include ActiveModel::Serializer::Cache |
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.
So, my suggestion was to really move the code out of the former class. Instead, you just moved it to a new file, but the code is still inside the Adapter.
In my opinion, the main problem with this approach is that you can't unit test the code you added, but only have integration tests, which aren't really good for refactoring the code later. My suggestion was to use composition, or in other words, call the cache class such as:
def cached_object
CachedResource.new(serializer).load
end
With that, you can unit test CachedResource
separately. Besides that, the adapter has a huge amount of methods doing things that are not the adapter's responsibility.
What do you think? cc @guilleiguaran
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.
Got it. Indeed. Serialization_test is getting too big.
Besides that, the adapter has a huge amount of methods doing things that are not the adapter's responsibility.
Convinced me
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.
@kurko, to avoid performance loss I'll just remove the fragment cache logic, that correspond for 90% of all code, agreed?:
def cached_object
klass = serializer.class
if klass._cache && !klass._cache_only && !klass._cache_except
_cache_key = (klass._cache_key) ? "#{klass._cache_key}/#{serializer.object.id}-#{serializer.object.updated_at}" : serializer.object.cache_key
klass._cache.fetch(_cache_key, klass._cache_options) do
yield
end
elsif klass._cache_only && !klass._cache_except || !klass._cache_only && klass._cache_except
FragmentCache.new(self, serializer, @options, @root).fetch
else
yield
end
end
There will be no other private method (related to cache) than cache_object. It will avoid pass blocks across function and keep it concise. What you think?
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.
Sounds good, that's a good start. I don't think, though, that another class will result in any significant loss.
On another not, I think we need to name things better here; we need to better group the code. For instance, I have no idea from a glance what the following is:
klass._cache_only && !klass._cache_except || !klass._cache_only && klass._cache_except
I'd rather have
def cached_object
# ...
elsif fragment_cache?
FragmentCache.new(self, serializer, @options, @root).fetch
else
#...
end
private
def fragment_cache?
klass._cache_only && !klass._cache_except || !klass._cache_only && klass._cache_except
end
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.
The loss would result of passing the yield block to order functions. But yeah it's nothing big.
Agreed. Even cached_object
isn't a good name, I'm working on this too.
@joaomdmoura what can I do to help here? |
@joaomdmoura looks awesome. 😍 |
@joshsmith tks man, but there are just some details missing, I'm working on it already 😄 but there are some improvements that I would like to work on in the near future that you can focus too, like the @thibaudgg suggestion. @thibaudgg Tks! Unfortunately the new cache feature does not use |
@joaomdmoura |
a96cbe7
to
966c233
Compare
@kurko, It's done.
|
🍻 |
Great work here, @joaomdmoura! Before merging, I want to spend more time with it. I'll deploy it to our staging env and see how it behaves and whether I can catch bugs or not. I'd advise you doing the same over there :D |
|
||
serializers = {cached: cached, non_cached: non_cached} | ||
cached_attributes_and_association(klass, serializers) | ||
return serializers |
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.
There's no need for this return
.
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.
Yeah, I know, I just felt that I should make it explicitly.
Never mind I'm removing it. 😝
966c233
to
ed32db4
Compare
@@ -32,8 +35,36 @@ def self.adapter_class(adapter) | |||
"ActiveModel::Serializer::Adapter::#{adapter.to_s.classify}".safe_constantize | |||
end | |||
|
|||
def fragment_cache(*args) |
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.
@kurko abstracted method. Duck typing strategy.
👍 |
Nice! |
163aa1d
to
f7fc56f
Compare
I tried this in an app and couldn't make it work. Basically, when I add
My serializer is like this:
If I leave When fixing, please make sure that both virtual attributes and associations are working. Great work, by the way. |
Needs to be rebased 😬 |
2f7490c
to
b8c5af4
Compare
@kurko fixed, new tests written, rebased and squashed. 😄 |
FYI, I'm trying to backport this PR into a fork that supports RC1 so I can test this in my app. If it works there, I'll merge it. This is so central to AMS; it has to work perfectly. |
private | ||
|
||
def cache_check(serializer) | ||
@serializer = serializer |
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.
This mutation is breaking a test in https://github.com/ride/active_model_serializers/blob/master-rc1-with-fragment-cache/test/action_controller/json_api_linked_test.rb#L146-L150 (it's a branch which I'm backporting this into RC1 because that's what my company is using).
Basically, halfway through the serialization (within a execution of serializable_hash
), @serializer
, which was PostSerializer
, becomes AuthorSerializer
after this. That same test is passing in this PR, so I'm not really sure what's going on, but regardless of that, mutating a variable that's used to figure out how to serialize stuff is bad and can lead to nasty behaviors in the future. If we're going to mutate, we need to simply instantiate a new object then or make sure the serialization has finished.
@joaomdmoura if you're available, let's discuss this over chat :)
b8c5af4
to
a2023b9
Compare
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.
a2023b9
to
792fb8a
Compare
@joaomdmoura first of all, great work here! This got better and better every iterations. I tried this in my app and tests are green 😁 Update: here are the numbers I got:
|
Great @kurko I'm sad about the fragment cache result, but as we discussed it would be really useful for attributes that involves some kind of logic, as virtual attributes on the serializer itself. Would be great to benchmark this case too 😄 But there is some improvement we can work on to optimize fragment cache speed, once benchmark tests are done will be way easier to keep track of it. |
Awesome. I'm testing it and if I don't find issues in a day or two, I'll merge this 😄 |
Any update on this? Really looking forward to this :) |
So, does any one have any problems with this? If not, I'm merging this pretty soon (likely tomorrow). |
@kurko can you paste the code you used for benchmarks in a gist? |
# both fragment
cache only: [:field1, :field2, :etc]
# and not fragment
cache
# then, in an integration test:
Benchmark.bm do |x|
x.report do
1000.times do
get "/users/#{user.id}", nil
end
end
end |
\o/ :laughing: |
end | ||
|
||
def fragment_serializer(name, klass) | ||
cached = "#{name.capitalize}CachedSerializer" |
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.
If different serializers share the same model, but has totally different attributes, would their caches collide, even if the key is different? I believe i see it on my machine. If you believe this could happen, i can present those tests to you, but i need time to set it up.
maybe it should go something like cached = "#{serializer.name}CachedSerializer"
?
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.
Yeap, I think it could happen. Not the cache_key itself, because it takes the file path into account but the Class name would colide.
I'll check it on the master codebase, create a new issue for that and update it.
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.
@joaomdmoura thanks, that would be great!
Here is the stack trace of the error i've got, when the classes collide, in case you would like to look at it. The latter called serializer tries to look for an attribute that was held by the first serializer, but not in the latter. Am i abusing the way serializers should be used, if i'm the first one who's got that error?
Failure/Error: get '/markers/local.json?coordinates=55.75,36.00&limit=10'
NoMethodError:
undefined method `name' for #<MarkerDistanceSerializer:0x007f80339b6eb8>
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer.rb:155:in `public_send'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer.rb:155:in `block in attributes'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer.rb:151:in `each'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer.rb:151:in `each_with_object'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer.rb:151:in `attributes'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:15:in `block in serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter.rb:51:in `block in cache_check'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:299:in `block in fetch'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:585:in `block in save_block_result_to_cache'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:547:in `block in instrument'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/notifications.rb:166:in `instrument'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:547:in `instrument'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:584:in `save_block_result_to_cache'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/cache.rb:299:in `fetch'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter.rb:50:in `cache_check'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:14:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/flatten_json.rb:6:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/fragment_cache.rb:27:in `fetch'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter.rb:54:in `cache_check'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:14:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/flatten_json.rb:6:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:10:in `block in serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/array_serializer.rb:6:in `each'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/array_serializer.rb:6:in `each'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:10:in `map'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/json.rb:10:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter/flatten_json.rb:6:in `serializable_hash'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/active_model/serializer/adapter.rb:24:in `as_json'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/json/encoding.rb:35:in `encode'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/json/encoding.rb:22:in `encode'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/core_ext/object/json.rb:37:in `to_json_with_active_support_encoder'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/renderers.rb:116:in `block in <module:Renderers>'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/bundler/gems/active_model_serializers-e7d3323d2352/lib/action_controller/serialization.rb:51:in `block (2 levels) in <module:Serialization>'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/renderers.rb:45:in `block in _render_to_body_with_renderer'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/renderers.rb:41:in `_render_to_body_with_renderer'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/renderers.rb:37:in `render_to_body'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/abstract_controller/rendering.rb:25:in `render'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/rendering.rb:16:in `render'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/instrumentation.rb:44:in `block (2 levels) in render'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/core_ext/benchmark.rb:12:in `block in ms'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.3/lib/active_support/core_ext/benchmark.rb:12:in `ms'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/instrumentation.rb:44:in `block in render'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/instrumentation.rb:87:in `cleanup_view_runtime'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/railties/controller_runtime.rb:25:in `cleanup_view_runtime'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/instrumentation.rb:43:in `render'
# /Users/antonmurygin/.rvm/gems/ruby-2.2.2/gems/remotipart-1.2.1/lib/remotipart/render_overrides.rb:14:in `render_with_remotipart'
# ./app/controllers/markers_controller.rb:20:in `block (2 levels) in local'
@kurko I'm trying to write a benchmark with positive results for caching but haven't been able to show differences with it on or off. I'm probably missing something. I see you had a working benchmark at one point. help?
|
@beauby The number in the comment #810 (comment) were a quote of @kurko . I haven't been able to replicate much.. see #1393 |
It's an upgrade based on the new Cache implementation #693.
Referenced: #802
It allows to use the Rails conventions to cache specific attributes or associations, as you will see in the example ahead.
It's optimized to re-use the cache in ever way as possible, in order to do this, the cached data will shared and resued on different methods, also the Serializers that are cached will be used when referenced as relationship at other serializer.
If there is some API endpoint that shouldn't be fully cached, you can still optimise it, using Fragment Cache on the attributes and relationships that you want to cache.
You can define the attributes by using
only
orexcept
option on cache mehtod.Example:
It also work when defining a key to an attribute, in this case you should use the original name of the attribute (no worries, the key still will work):