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/
signature.asc
Description: This is a digitally signed message part
