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

Avoid calling define_method in CollectionProxy#scope #10058

Merged
merged 1 commit into from Apr 5, 2013

Conversation

Projects
None yet
5 participants
@jamesgolick
Contributor

jamesgolick commented Apr 2, 2013

Calling define_method busts the method cache, which has a major performance penalty. This refactoring avoids that in CollectionProxy#scope.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik
Member

steveklabnik commented Apr 2, 2013

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 2, 2013

Member

@jonleighton you added this in 5da90b3 - any reason why it was done with define_method?

@jamesgolick does it bust the cache twice? Once for define_method and once for the extend inside the extending! method.

Member

pixeltrix commented Apr 2, 2013

@jonleighton you added this in 5da90b3 - any reason why it was done with define_method?

@jamesgolick does it bust the cache twice? Once for define_method and once for the extend inside the extending! method.

@jamesgolick

This comment has been minimized.

Show comment
Hide comment
@jamesgolick
Contributor

jamesgolick commented Apr 2, 2013

@pixeltrix yes.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Apr 3, 2013

Member

👍 to merge this, let's wait for @jonleighton's comment

Member

spastorino commented Apr 3, 2013

👍 to merge this, let's wait for @jonleighton's comment

@ghost ghost assigned jonleighton Apr 3, 2013

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Apr 5, 2013

Member

I used define_method because I didn't want to muddy Relation with concerns about associations. However I think the performance impact of this justifies the muddying for now. (And potential refactoring in the future - we have talked about making an AssociationRelation subclass for these things.)

Member

jonleighton commented Apr 5, 2013

I used define_method because I didn't want to muddy Relation with concerns about associations. However I think the performance impact of this justifies the muddying for now. (And potential refactoring in the future - we have talked about making an AssociationRelation subclass for these things.)

jonleighton added a commit that referenced this pull request Apr 5, 2013

Merge pull request #10058 from jamesgolick/master
Avoid calling define_method in CollectionProxy#scope

@jonleighton jonleighton merged commit bab0e28 into rails:master Apr 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment