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.

Sign up for GitHub

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 serializers for class #833

Merged
merged 4 commits into from
Mar 13, 2015
Merged

Cache serializers for class #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fb5609
cache the serializers for a class
lsylvester Mar 10, 2015
2b0c5ee
clear the cache between requests
lsylvester Mar 10, 2015
980d1ce
add explicit thread_safe dependency
Mar 11, 2015
270b312
remove the thread_safe dependency. Relay on rails for this
Mar 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'thread_safe'

module ActiveModel
class Serializer
extend ActiveSupport::Autoload
Expand Down Expand Up @@ -199,16 +201,22 @@ def serializer_from_options(options)
opts
end

def self.serializers_cache
@serializers_cache ||= ThreadSafe::Cache.new
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with this. Can you explain what this does and whether there are any potential side effects?

Copy link
Member

Choose a reason for hiding this comment

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

btw it would generate a new dependency with thread_safe gem right?
Is there some reason to not use ActionController::Base.cache_store

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's correct, and I see no reference to ThreadSafe elsewhere. That concerns me a little.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Threadsafe is a dependancy of ActiveSupport, so it is already indirectly as dependancy. I can add it is explicitly as a dependancy.

The reason that to use it instead of ActionController::Base.cache_store is that is in purely in memory so is faster than going to an external cache like memcached which ActionController::Base.cache_store would do.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Well, it seem cool for me as we are talking about small things.
Can it cause some problem to not-quit-amazing-servers with less memory ir is it derisive?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed ActiveSupport::TimeZone.zones_map in Rails 4.1 is using ThreadSafe::Cache, as you can see here and here.
Okay, I think it's enough to make me feel comfortable about it. cc/ @kurko @joshsmith @guilleiguaran

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree then, but really appreciate the discussion on it. Thanks for obliging us @lsylvester! 🏆

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the hardcoded dependency? That way, if Rails removes it AMS tests will explode and we'll me notified that we need to do something about it (either leave it or replace it, in case there is a better alternative).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kurko the dependency has been removed.

Copy link
Member

Choose a reason for hiding this comment

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

👍 great work @lsylvester
Thank you!

end

private

def self.get_serializer_for(klass)
serializer_class_name = "#{klass.name}Serializer"
serializer_class = serializer_class_name.safe_constantize

if serializer_class
serializer_class
elsif klass.superclass
get_serializer_for(klass.superclass)
serializers_cache.fetch_or_store(klass) do
serializer_class_name = "#{klass.name}Serializer"
serializer_class = serializer_class_name.safe_constantize

if serializer_class
serializer_class
elsif klass.superclass
get_serializer_for(klass.superclass)
end
end
end

Expand Down
3 changes: 3 additions & 0 deletions lib/active_model_serializers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

ActiveSupport.on_load(:action_controller) do
include ::ActionController::Serialization
ActionDispatch::Reloader.to_prepare do
ActiveModel::Serializer.serializers_cache.clear
end
end
rescue LoadError
# rails not installed, continuing
Expand Down