Performance of "delegate"

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)

I am going to just repost what I said in GH-2711...

TL;DR - I agree with you, but others need convincing.

@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

I 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

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.

@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.

@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.

@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

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.

The deprecation stuff is happening at
https://github.com/dasch/rails/tree/delegate-deprecation

Comments are welcome - the code can be tricky.

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

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?