-
Notifications
You must be signed in to change notification settings - Fork 23
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
Remove active attr typecasting #43
Remove active attr typecasting #43
Conversation
👍 |
@@ -1,5 +1,13 @@ | |||
module ActiveRemote | |||
module Attributes | |||
def attributes |
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.
Why is this necessary? Not opposed, just want to understand.
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.
active_attr has a very strage mechanism for internal attribute storage, and there is an attributes_map thing that is not-so-great for performance
https://github.com/cgriego/active_attr/blob/master/lib/active_attr/attributes.rb#L60
This just bypasses all of that. I would really like to move to the structure active_record uses for attribute storage
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/attribute_set.rb
The reason attributes has to be initialized like this is because when you declare an attribute at the class level, a new instance is instantiated and then calls respond_to?
This is to check for any methods dispatched via method_missing
.
https://github.com/cgriego/active_attr/blob/master/lib/active_attr/attributes.rb#L254
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.
👍
Regarding the performance of the typecasters, I feel that the best way to improve their performance is to skip them entirely for data coming from the data adapters. Since the data adapters often already handle typecasting, if the attribute storage mechanism allowed a way to write attributes without typecasting the data adapters could skip this code. I simply brought over the existing active_attr typecasters in an attempt to make sure we are not introducing breaking changes in this release. So with that in mind, I would prefer to leave the typecasters alone for now. I think with a bit of work the typecasters will be optimized out of reads entirely. |
This removes the active_attr typecasting module and and implements our own. Instead of typecasting at the read time of an attribute (or attributes), this moves the typecasting to write time.
This is a substantial performance improvement whenever
attributes
, orrespond_to?
is called on an instance, we do not have to typecast every attribute.I imported the active_attr typecasters and also kept their same dsl so this is backwards compatible.
In the future I think the attribute store should allow attributes to be written without typecasting, so that we can skip that step when getting data from an adapter, which has likely already done the typecasting. Currently the attribute store is just a dumb hash, but I think that should be improved.