Fix up caching, especially fragment_cache #1781
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -167,7 +167,7 @@ def serializable_hash(adapter_options = nil, options = {}, adapter_instance = se | |||
| adapter_options ||= {} | |||
| options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options) | |||
| cached_attributes = adapter_options[:cached_attributes] ||= {} | |||
| resource = cached_attributes(options[:fields], cached_attributes, adapter_instance) | |||
| resource = fetch_attributes(options[:fields], cached_attributes, adapter_instance) | |||
There was a problem hiding this comment.
otherwise we have a method that takes an argument (a hash) of the same name
| self.class._cache_options ||= {} | ||
| self.class._cache_options[:key] = self.class._cache_key if self.class._cache_key | ||
|
|
||
| cached_serializer = _get_or_create_fragment_cached_serializer(serializer_class_name) | ||
| cached_serializer = _get_or_create_fragment_cached_serializer | ||
| cached_hash = ActiveModelSerializers::SerializableResource.new( | ||
| object, | ||
| serializer: cached_serializer, | ||
| adapter: adapter_instance.class | ||
| ).serializable_hash |
There was a problem hiding this comment.
first pass at trying to remove the extra class failed, so I left it here
something like
cached_hash = cached_attributes.fetch(key) do
self.class.cache_store.fetch(key) do
attributes(fields, true)
end
end(added cached_attributes to the method params and removed all references to _fragmented since there would be no more special classes)
|
It's funny when code coverage drops because there's less code :) |
| @@ -208,46 +199,44 @@ def object_cache_key(serializer, adapter_instance) | |||
| end | |||
| end | |||
|
|
|||
| def cached_attributes(fields, cached_attributes, adapter_instance) | |||
| ### INSTANCE METHODS | |||
| def fetch_attributes(fields, cached_attributes, adapter_instance) | |||
| if self.class.cache_enabled? | |||
There was a problem hiding this comment.
I think we could have klass = self.class here since we use it many times.
There was a problem hiding this comment.
Yeah. Ultimately, I'd like to move a lot of this into a class that lives outside of the serializer. I started to write that and had a serializer_class attribute
| cache_check(adapter_instance) do | ||
| attributes(fields) | ||
| self.class.cache_store.fetch(key, self.class._cache_options) do | ||
| attributes(fields, true) |
There was a problem hiding this comment.
passing true since we always want to reload the attributes in a cache block
- on association, fix up assocation logic - on attribute
bf4
added a commit
to bf4/active_model_serializers
that referenced
this pull request
Jun 8, 2016
This was referenced Jun 8, 2016
bf4
added a commit
to bf4/active_model_serializers
that referenced
this pull request
Jun 8, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follows from #1766 and #1684
Benchmark results look good:. Fragment cache went from in #1766
1729 objs/iteration to 1382, and from 716 ips to 767.
It's still not as good as caching or non-caching, but at least we're
seeing some actual performance improvements now.
Related:
step 1 removing the non caching fragment cache class
step 2 removing the caching fragment serializer class