Cache serializers for class #833
Conversation
|
👍 It seems good to me. @joshsmith ? |
| @@ -199,16 +199,22 @@ def serializer_from_options(options) | |||
| opts | |||
| end | |||
|
|
|||
| def self.serializers_cache | |||
| @serializers_cache ||= ThreadSafe::Cache.new | |||
There was a problem hiding this comment.
I'm not very familiar with this. Can you explain what this does and whether there are any potential side effects?
There was a problem hiding this comment.
btw it would generate a new dependency with thread_safe gem right?
Is there some reason to not use ActionController::Base.cache_store
There was a problem hiding this comment.
I think that's correct, and I see no reference to ThreadSafe elsewhere. That concerns me a little.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I agree then, but really appreciate the discussion on it. Thanks for obliging us @lsylvester! 🏆
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@kurko the dependency has been removed.
There was a problem hiding this comment.
👍 great work @lsylvester
Thank you!
|
Summoning @guilleiguaran. |
lsylvester
force-pushed
the
cache-serializers-for-class
branch
from
ca384e6 to
270b312
Compare
Currently, each time a model is serializer is tries to look up the Serializer class using
safe_constantize. This can be slow when serializing a large number of models.This adds a cache to return the serializer class for each class, which reduced the amount of time looking up the serializer classes.