Performance of "delegate"

rubyonrails-core
1

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)

2

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

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

3

@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

4

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

5

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.

6

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

7

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

8

@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

9

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.

10

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

Comments are welcome - the code can be tricky.

11

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

12

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.

13

As you say, this would complicate things quite a bit. What happens when the specified arity is wrong?