Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refactor Module.delegate #2255

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

bogdan commented Jul 25, 2011

Current implementation of Module.delegate have some evil in constructing ruby code too match.
This patch rewrites the method to have minimal amount of eval, that I suppose always a good idea e.g. more code reused, less strings to construct.

Diff says it all.

@bogdan bogdan Refactor Module.delegate
Rewrite code to have minimum amount of eval
d8e264b
Contributor

josevalim commented Jul 25, 2011

Unfortunately this has the performance downside of an extra method dispatch. /cc @tenderlove

Contributor

dmathieu commented Jul 25, 2011

Delegation is a very generic term and you set it a first-level class. That's gonna be a problem for many apps.
You should use ActiveSupport::Delegation.

Contributor

bogdan commented Jul 25, 2011

@josevalim Not sure I got you right. Did you mean that parse method call Delegation.perform in the eval string is slow or method call itself is slow?

@dmathieu will fix

Owner

tenderlove commented Jul 25, 2011

@josevalim using delegate is already slow. I don't think an extra method call would make much difference.

I think I'm OK with this patch if Delegation is properly namespaced and has a # :nodoc: directive.

Contributor

bogdan commented Jul 26, 2011

Properly namespaced in #2275.

@bogdan bogdan closed this Jul 26, 2011

Member

jonleighton commented Jul 26, 2011

I share the concerns of @josevalim. Just because it's currently slow doesn't mean that we should resign ourselves to it always being slow.

I agree the code is too complex, but I wonder if it could instead be refactored into a helper class that would build the class-eval'ed string.

(Incidentally, before I went away last week I was working on a speed improvement for delegate that gets rid of the send call in most cases, but it's complicated because it needs to handle edge cases and it requires deprecating the use of delegate on non-public methods. [It's not done yet...])

Contributor

bogdan commented Jul 26, 2011

My main concern was initialization performance that is increased by 33% by this patch.

@jonleighton is right - execution performance is slower. But benefits of improving execution performance is really low comparing to execution time of the method itself: Guys that goes that deep into the performance issue should not use ActiveSupport at all.

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