Performance of "delegate"

[Daniel Schierbeck]

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)

[Jon Leighton]

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

--
http://jonathanleighton.com/

[Daniel Schierbeck]

@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

[Jon Leighton]

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

--
http://jonathanleighton.com/

[Xavier Noria]

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.

[Daniel Schierbeck]

@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 Leighton]

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/

[Daniel Schierbeck]

@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

--
Med venlig hilsen
Daniel Schierbeck

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

On Mon, Aug 29, 2011 at 2:42 PM, Jon Leighton <j...@jonathanleighton.com> wrote:

--

[Daniel Schierbeck]

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

Comments are welcome - the code can be tricky.

[Daniel Schierbeck]

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

[matthewr...@gmail.com]

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. 

[Daniel Schierbeck]

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