Hi guys,
I recently discussed the `delegate` method with @tenderlove in
connection with GitHub issue #2711. I wished to replace
a series of "manual" delegations (using method declarations) with a
single call to `delegate`. Mr. @tenderlove correctly
pointed out that this would incur a deterioration of performance.
The current implementation of `delegate` is basically this, sans all
the bells and whistles:
def delegate(target, method)
class_eval(<<-RUBY)
def #{method}(*args, &block)
#{target}.__send__(:#{method}, *args, &block)
end
RUBY
end
He also identified four run-time issues which hurt performance
(paraphrased):
1. Stack depth impacts GC time
2. Paying an extra method call `__send__`
3. `*args` contraction (we must build an array that is just GC''d)
4. Splatting the args back to the `__send__`
While I cannot currently find a way to improve 3 and 4, I believe we
can alleviate the problems caused by the increased
stack depth and the overhead of the extra method call. These problems
both arise due to the use of `__send__`. Eliminating
the call yields:
def delegate(target, method)
class_eval(<<-RUBY)
def #{method}(*args, &block)
#{target}.#{method}(*args, &block)
end
RUBY
end
I ran a benchmark of the different implementations (https://
gist.github.com/1178156) using MRI versions 1.8.7 and 1.9.2.
The "method" row is the baseline, i.e. a manual delegation using a
full method definition. I''d love to see more people run
the benchmark to see how the results stack up.
Using MRI 1.8.7:
user system total real
method 8.460000 0.010000 8.470000 ( 8.477583)
old 12.960000 0.010000 12.970000 ( 12.978188)
new 9.350000 0.010000 9.360000 ( 9.365799)
Using MRI 1.9.2:
user system total real
method 4.340000 0.000000 4.340000 ( 4.349549)
old 4.670000 0.000000 4.670000 ( 4.664780)
new 4.480000 0.000000 4.480000 ( 4.477026)
The new implementation is clearly faster than the old, especially on
1.8.7.
There are, however, two caveats to this new implementation:
1. Writer methods (those ending in "=") fail spectacularly. This is
due to the fact that you cannot directly call such a
method with more than one argument, i.e. `foo.bar=(1, 2)` is not
syntactically valid, while `foo.__send__(:bar, 1, 2)`
is. This can be solved by defining the argument list differently
for such methods - which would also benefit performance!
2. The semantics of `delegate` are slightly changed: it is no longer
possible to delegate to private methods.
The second issue is something that should be discussed here. I''m
biased towards loose coupling, and therefore believe
calling private methods on another object is inherently evil. It
*will* be a problem with regards to backwards compatibility,
assuming people actually use `delegate` for such a nefarious purpose.
I hope people will be interested in discussing this topic, as I think
`delegate` is an important part of the "plumbing" of
Rails, and deserves all the optimization we can muster. This is
especially true when we seize to use it internally because
it is too slow.
Cheers,
Daniel Schierbeck (@dasch)
--
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
I am going to just repost what I said in GH-2711... TL;DR - I agree with you, but others need convincing. -------------- @dasch I tried to change this recently, and subsequently reverted in 8ba491a[1]. See discussion in aa1d1e4[2]. As you say, we have to deal with writer methods to make this work. My implementation dealt with this by backporting public_send for 1.8. However, the backport needed some more work and in any case would have had performance difficulties I think. It would be possible to do a special case for writer methods, but they wouldn''t be able to deal with more than one argument, without a backport of public_send. IMO it would be best to just not let delegate work with writer methods + multiple args (it''s a very very edge case). But anyway, given the opposition to this change (see discussion in the commit linked) and the various complications with implementing it, I lost interest and reverted. By all means feel free to try to persuade people and suggest an implementation, but I am just letting you know that I''ve already been down the road and gave up ;) [1] https://github.com/rails/rails/commit/8ba491acc31bf08cf63a83ea0a3c314c52cd020f [2] https://github.com/rails/rails/commit/aa1d1e4962ba218f34defd0e7f0b665c795eb12b -------------- On Mon, 2011-08-29 at 03:59 -0700, Daniel Schierbeck wrote:> Hi guys, > > I recently discussed the `delegate` method with @tenderlove in > connection with GitHub issue #2711. I wished to replace > a series of "manual" delegations (using method declarations) with a > single call to `delegate`. Mr. @tenderlove correctly > pointed out that this would incur a deterioration of performance. > > The current implementation of `delegate` is basically this, sans all > the bells and whistles: > > def delegate(target, method) > class_eval(<<-RUBY) > def #{method}(*args, &block) > #{target}.__send__(:#{method}, *args, &block) > end > RUBY > end > > He also identified four run-time issues which hurt performance > (paraphrased): > > 1. Stack depth impacts GC time > 2. Paying an extra method call `__send__` > 3. `*args` contraction (we must build an array that is just GC''d) > 4. Splatting the args back to the `__send__` > > While I cannot currently find a way to improve 3 and 4, I believe we > can alleviate the problems caused by the increased > stack depth and the overhead of the extra method call. These problems > both arise due to the use of `__send__`. Eliminating > the call yields: > > def delegate(target, method) > class_eval(<<-RUBY) > def #{method}(*args, &block) > #{target}.#{method}(*args, &block) > end > RUBY > end > > > I ran a benchmark of the different implementations (https:// > gist.github.com/1178156) using MRI versions 1.8.7 and 1.9.2. > The "method" row is the baseline, i.e. a manual delegation using a > full method definition. I''d love to see more people run > the benchmark to see how the results stack up. > > Using MRI 1.8.7: > > user system total real > method 8.460000 0.010000 8.470000 ( 8.477583) > old 12.960000 0.010000 12.970000 ( 12.978188) > new 9.350000 0.010000 9.360000 ( 9.365799) > > > Using MRI 1.9.2: > > user system total real > method 4.340000 0.000000 4.340000 ( 4.349549) > old 4.670000 0.000000 4.670000 ( 4.664780) > new 4.480000 0.000000 4.480000 ( 4.477026) > > The new implementation is clearly faster than the old, especially on > 1.8.7. > > There are, however, two caveats to this new implementation: > > 1. Writer methods (those ending in "=") fail spectacularly. This is > due to the fact that you cannot directly call such a > method with more than one argument, i.e. `foo.bar=(1, 2)` is not > syntactically valid, while `foo.__send__(:bar, 1, 2)` > is. This can be solved by defining the argument list differently > for such methods - which would also benefit performance! > 2. The semantics of `delegate` are slightly changed: it is no longer > possible to delegate to private methods. > > The second issue is something that should be discussed here. I''m > biased towards loose coupling, and therefore believe > calling private methods on another object is inherently evil. It > *will* be a problem with regards to backwards compatibility, > assuming people actually use `delegate` for such a nefarious purpose. > > I hope people will be interested in discussing this topic, as I think > `delegate` is an important part of the "plumbing" of > Rails, and deserves all the optimization we can muster. This is > especially true when we seize to use it internally because > it is too slow. > > > Cheers, > Daniel Schierbeck (@dasch) >-- http://jonathanleighton.com/
@jonleighton thanks for chiming in. I really believe that the basic constructs provided by ActiveSupport should uphold the rules of OOP - if you need to delegate to a private method nothing is stopping you from going the manual route. I seem to have fixed the performance issues, although I completely removed the ability to delegate to private methods, rather than deprecating it like you did. I''ve created a pull request so that the changes are more apparent: https://github.com/rails/rails/pull/2733 -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
On Mon, 2011-08-29 at 04:18 -0700, Daniel Schierbeck wrote:> @jonleighton thanks for chiming in. I really believe that the basic > constructs provided by ActiveSupport should uphold the rules of OOP - > if you need to delegate to a private method nothing is stopping you > from going the manual route. > > I seem to have fixed the performance issues, although I completely > removed the ability to delegate to private methods, rather than > deprecating it like you did. > > I''ve created a pull request so that the changes are more apparent: > https://github.com/rails/rails/pull/2733I removed the ability in master, and deprecated it in 3-1-stable. If we make this change, we do need to give people an upgrade path, so there does need to be a deprecation before the change is actually made. One feature of your implementation is that the following will not work: class Foo delegate :foo=, :to => :bar end foo.send(:foo=, 1, 2) # => will do ''foo.bar.foo = 1'', not ''foo.bar.send(:foo=, 1, 2)'' I think this is reasonable because this is a quite esoteric usage of a writer method. However, we should raise ArgumentError if there are multiple arguments in this case, and again we would need to provide a deprecation. I think checking args.length for writer methods should be performant enough for the deprecation. Then, in the future implementation, we could just define writer methods with no splat, so Ruby would take care of raising the ArgumentError. Jon -- http://jonathanleighton.com/
Regarding private methods, I expressed my point of view in Jon''s. delegate is just a convenience method, a little utility that 1) saves the user from writing the delegation by hand 2) it is declarative, which is a plus in my view, intention is more clear Since I can delegate to private methods, and delegate is just a macro, I see no reason why it should impose any arbitrary restriction like visibility. Conceptually. That being said, if that was dropped for the benefit of performance, I''d personally buy that. In order not to put a penalty on all calls, you need to write your own wrappers for private methods. Sounds reasonable to me. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
@Jon: I could change the implementation to check `args.length` and
raise ArgumentError if the result is greater than 1. But wouldn''t it
suffice to simply define the writer methods with only one argument?
@Xavier: I think users perceive `delegate` as a shorthand for:
def foo
@target.foo
end
and not
def foo
@target.__send__(:foo)
end
so I actually believe the proposed semantics more closely resembles
the perception. That''s just my point of view, of course.
--
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
On Mon, 2011-08-29 at 14:16 +0200, Daniel Schierbeck wrote:> @Jon: I could change the implementation to check `args.length` and > raise ArgumentError if the result is greater than 1. But wouldn''t it > suffice to simply define the writer methods with only one argument?Yes, that''s what I meant. We need a patch against 3-1-stable that: * Calls @target.foo, rescues NoMethodError, tries @target.__send__(:foo) and if that works, shows a deprecation warning. (The reason for this implementation is that there should be no performance hit in the public case.) * Shows a deprecation warning if args.length > 1 && method_name.ends_with?(''='') Then, we need the patch against master to just define def foo=(bar) (no splat), when method_name.ends_with?(''=''). If the deprecation in 3-1-stable is too involved, we might need to think about deprecating in 3.2 and removing in 3.3. But let''s see the code first. Also note that this deprecation won''t get into 3.1.0, so it would be a new deprecation in 3.1.1. (Some may object to this, but I think it''s acceptable personally.)> @Xavier: I think users perceive `delegate` as a shorthand for: > > def foo > @target.foo > end > > and not > > def foo > @target.__send__(:foo) > end > > so I actually believe the proposed semantics more closely resembles > the perception. That''s just my point of view, of course.Yes, this is my perspective as well. To me the POLS is that ''delegate'' is a macro for the former, which does not support non-public methods. -- http://jonathanleighton.com/
@Jon: I''ve updated my patch at https://github.com/rails/rails/pull/2733/files The new version should play well with []= methods. If the change is accepted I will be perfectly willing to implement a deprecation patch for 3.1. Should I begin now, or is the issue still contested? Cheers, Daniel Schierbeck On Mon, Aug 29, 2011 at 2:42 PM, Jon Leighton <j@jonathanleighton.com> wrote:> On Mon, 2011-08-29 at 14:16 +0200, Daniel Schierbeck wrote: >> @Jon: I could change the implementation to check `args.length` and >> raise ArgumentError if the result is greater than 1. But wouldn''t it >> suffice to simply define the writer methods with only one argument? > > Yes, that''s what I meant. > > We need a patch against 3-1-stable that: > > * Calls @target.foo, rescues NoMethodError, tries @target.__send__(:foo) > and if that works, shows a deprecation warning. (The reason for this > implementation is that there should be no performance hit in the public > case.) > * Shows a deprecation warning if args.length > 1 && > method_name.ends_with?(''='') > > Then, we need the patch against master to just define def foo=(bar) (no > splat), when method_name.ends_with?(''=''). > > If the deprecation in 3-1-stable is too involved, we might need to think > about deprecating in 3.2 and removing in 3.3. But let''s see the code > first. Also note that this deprecation won''t get into 3.1.0, so it would > be a new deprecation in 3.1.1. (Some may object to this, but I think > it''s acceptable personally.) > >> @Xavier: I think users perceive `delegate` as a shorthand for: >> >> def foo >> @target.foo >> end >> >> and not >> >> def foo >> @target.__send__(:foo) >> end >> >> so I actually believe the proposed semantics more closely resembles >> the perception. That''s just my point of view, of course. > > Yes, this is my perspective as well. To me the POLS is that ''delegate'' > is a macro for the former, which does not support non-public methods. > > -- > http://jonathanleighton.com/ >-- Med venlig hilsen Daniel Schierbeck -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
I''ve begun work on the patch against 3-1-stable, but it''s not trivial. For one, simply evaling target.method=(*args, &block) throws a syntax error. I''ll keep working on it. On Mon, Aug 29, 2011 at 2:42 PM, Jon Leighton <j@jonathanleighton.com> wrote:> On Mon, 2011-08-29 at 14:16 +0200, Daniel Schierbeck wrote: >> @Jon: I could change the implementation to check `args.length` and >> raise ArgumentError if the result is greater than 1. But wouldn''t it >> suffice to simply define the writer methods with only one argument? > > Yes, that''s what I meant. > > We need a patch against 3-1-stable that: > > * Calls @target.foo, rescues NoMethodError, tries @target.__send__(:foo) > and if that works, shows a deprecation warning. (The reason for this > implementation is that there should be no performance hit in the public > case.) > * Shows a deprecation warning if args.length > 1 && > method_name.ends_with?(''='') > > Then, we need the patch against master to just define def foo=(bar) (no > splat), when method_name.ends_with?(''=''). > > If the deprecation in 3-1-stable is too involved, we might need to think > about deprecating in 3.2 and removing in 3.3. But let''s see the code > first. Also note that this deprecation won''t get into 3.1.0, so it would > be a new deprecation in 3.1.1. (Some may object to this, but I think > it''s acceptable personally.) > >> @Xavier: I think users perceive `delegate` as a shorthand for: >> >> def foo >> @target.foo >> end >> >> and not >> >> def foo >> @target.__send__(:foo) >> end >> >> so I actually believe the proposed semantics more closely resembles >> the perception. That''s just my point of view, of course. > > Yes, this is my perspective as well. To me the POLS is that ''delegate'' > is a macro for the former, which does not support non-public methods. > > -- > http://jonathanleighton.com/ >-- Med venlig hilsen Daniel Schierbeck -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
The deprecation stuff is happening at https://github.com/dasch/rails/tree/delegate-deprecation Comments are welcome - the code can be tricky. On Mon, Aug 29, 2011 at 3:34 PM, Daniel Schierbeck <daniel.schierbeck@gmail.com> wrote:> I''ve begun work on the patch against 3-1-stable, but it''s not trivial. > For one, simply evaling > > target.method=(*args, &block) > > throws a syntax error. I''ll keep working on it. > > On Mon, Aug 29, 2011 at 2:42 PM, Jon Leighton <j@jonathanleighton.com> wrote: >> On Mon, 2011-08-29 at 14:16 +0200, Daniel Schierbeck wrote: >>> @Jon: I could change the implementation to check `args.length` and >>> raise ArgumentError if the result is greater than 1. But wouldn''t it >>> suffice to simply define the writer methods with only one argument? >> >> Yes, that''s what I meant. >> >> We need a patch against 3-1-stable that: >> >> * Calls @target.foo, rescues NoMethodError, tries @target.__send__(:foo) >> and if that works, shows a deprecation warning. (The reason for this >> implementation is that there should be no performance hit in the public >> case.) >> * Shows a deprecation warning if args.length > 1 && >> method_name.ends_with?(''='') >> >> Then, we need the patch against master to just define def foo=(bar) (no >> splat), when method_name.ends_with?(''=''). >> >> If the deprecation in 3-1-stable is too involved, we might need to think >> about deprecating in 3.2 and removing in 3.3. But let''s see the code >> first. Also note that this deprecation won''t get into 3.1.0, so it would >> be a new deprecation in 3.1.1. (Some may object to this, but I think >> it''s acceptable personally.) >> >>> @Xavier: I think users perceive `delegate` as a shorthand for: >>> >>> def foo >>> @target.foo >>> end >>> >>> and not >>> >>> def foo >>> @target.__send__(:foo) >>> end >>> >>> so I actually believe the proposed semantics more closely resembles >>> the perception. That''s just my point of view, of course. >> >> Yes, this is my perspective as well. To me the POLS is that ''delegate'' >> is a macro for the former, which does not support non-public methods. >> >> -- >> http://jonathanleighton.com/ >> > > > > -- > Med venlig hilsen > Daniel Schierbeck >-- Med venlig hilsen Daniel Schierbeck -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Okay, I''ve added a pull request targeting 3-1-stable: https://github.com/rails/rails/pull/2736 Please have a look. Cheers, Daniel Schierbeck -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
On Monday, August 29, 2011 11:59:56 AM UTC+1, Daniel Schierbeck wrote:> > > 3. `*args` contraction (we must build an array that is just GC''d) > 4. Splatting the args back to the `__send__` > > In fear of overcomplicating this functionality.You could avoid splatting in cases where you dont need it, via an extension of the api delegate :to_s, :to => :something, :arity => 0 where specifying an explicit arity, would do what you expect. This would involve a more complicated, multiple version approach to this functionality. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/uygkiZdUgPkJ. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
On Mon, Aug 29, 2011 at 6:34 PM, matthewrudyjacobs@gmail.com <matthewrudyjacobs@gmail.com> wrote:> On Monday, August 29, 2011 11:59:56 AM UTC+1, Daniel Schierbeck wrote: >> >> 3. `*args` contraction (we must build an array that is just GC''d) >> 4. Splatting the args back to the `__send__` >> > In fear of overcomplicating this functionality. > You could avoid splatting in cases where you dont need it, via an extension > of the api > delegate :to_s, :to => :something, :arity => 0 > where specifying an explicit arity, would do what you expect. > This would involve a more complicated, multiple version approach to this > functionality.As you say, this would complicate things quite a bit. What happens when the specified arity is wrong? -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.