Module#delegate refactoring to minimize ruby code eval #3290

Closed
wants to merge 1 commit into from

6 participants

@bogdan

Original discussion at: #2275.

Additions here: separated delegation method with allow_nil and without it as @jeremy suggested.

@dasch

This will make delegation slower.

@dmitriy-kiriyenko

-1.

  delegate :name, :to => :user

should be literally just another way of saying

  def name; user.name; end

It's currently already making stack one level deeper, also breaking the encapsulation because of __send__, and you suggest making it even more deep. Also, by removing one smell, you add two others - a duplication (to avoid it, we'll have to add even 1 more stack level and method dispatching), and two methods with seven formal parameters! (btw, such a long signature in two parallel methods itself is a duplication, that can't be avoided in this solution).

I've looked through discussions and can only agree with @josevalim and @jonleighton, that just because it's currently slow doesn't mean that we should resign ourselves to it always being slow.

This is not a good refactor, I think.

@josevalim
Ruby on Rails member

I am also -1 for this. We have some parts on ActiveSupport that relies considerably on class_eval for performance and I wouldn't like to change them. @jeremy, you've asked to rebase it, do you really want to merge it? holds a knife

@dasch

I've made delegate significantly faster in #2733. A migration path is provided in #2736, which deprecates delegation to private methods. @josevalim, will you take a look at the latter if I rebase it against master?

@josevalim
Ruby on Rails member

The problem is that the migration path is less than optimal. Maybe we could have two delegate versions, one called delegate only for public and delegate! that uses __send__ and work with protected + private.

@dasch

@josevalim: the problem is that this might cause existing apps to fail if they don't change delegate into delegate!. I'm okay with that, and it's a much simpler change, but it depends on you guys. I'd be more than happy to update #2733 and add back the old version with a bang on the end and a deprecation message. Would that be okay?

@dmitriy-kiriyenko

@josevalim, sounds nice! About delegate and delegate!. I'd +1.
@dasch, this is ok for me if it's stated in changelog and upgrade guides. Also, supposing everyone has good tests, we can catch the exception in delegate and re-throw it with a message "Delegation via delegate to private/protected methods is not supported anymore. Please, use delegate! instead."

#2733 is good, #2736 isn't. delegate! looks like a nice workaround.

@dasch

@dmitriy-kiriyenko catching the exceptions caused by calling a private method is much harder than you'd expect. Take a look at #2736 to see how many hoops you have to run through.

Can we move this discussion to #2733? I've pushed a new commit which adds back the old method with a "!" suffix and a deprecation warning.

@jeremy
Ruby on Rails member

This can be counterintuitive, actually. Generating tons of code puts extra pressure on the garbage collector. Consolidating code by using just a method call can relieve that, as we saw code generation for route helpers.

I'd like to see some benchmarks. The results in @bogdan's original pull request implied his refactor was actually faster. (Though he didn't present those results clearly. They may be reversed.)

From https://gist.github.com/1108920

ORIGINAL:
       user     system      total        real
   0.120000   0.010000   0.130000 (  0.132569)
   0.090000   0.000000   0.090000 (  0.096892)

REFACTORED:
       user     system      total        real
   0.010000   0.000000   0.010000 (  0.002482)
   0.000000   0.000000   0.000000 (  0.002776)

@bogdan, you didn't need to open a new pull request! This makes it hard to follow history.

@dasch

@jeremy: this post on the mailing list details the original reason why I created the pull request and also has a benchmark showing significant gains from removing the indirection, especially on MRI 1.9: http://groups.google.com/group/rubyonrails-core/browse_thread/thread/c47a7d7c93ae5eea?pli=1&fwc=1

@jonleighton
Ruby on Rails member

sigh this again.

I am -1 on this change. I am +1 on the following (this is pseudo code, don't flame me).

Rails 3.2

Generated code:

def name(*args, &block)
  user.name(*args, &block)
rescue NoMethodError => error
  ActiveSupport::Delegation.handle_error(object, name, *args, &block, error)
end

Where:

module ActiveSupport::Delegation
  def self.handle_error(object, name, *args, &block, error)
    if error caused by delegation
      result = object.__send__(name, *args, &block)
      warn "delegation like this is deprecated"
      result
    else
      raise error
    end
  end
end

For this to work, we also need to deprecate using delegate with methods that:

  1. Have more than 1 argument
  2. Have a name that ends in '='

Like this:

def name=(*args, &block)
  if args.length > 1 || block
    ActiveSupport::Delegation.handle_assignment_method(object, name, *args, &block)
  else
    user.name = args.first
  end
rescue NoMethodError => error
  ActiveSupport::Delegation.handle_error(object, name, *args, &block, error)
end

Rails 3.2 + 1

Generated code:

def name(*args, &block)
  user.name(*args, &block)
end

Assignment method:

def name=(arg)
  user.name = arg
end

Allow nil method:

def name(*args, &block)
  user && user.name(*args, &block)
end

Points to note

  • Generated code is kept to a minimum
  • The 3.2 version is only slow if you haven't dealt with the deprecation warning. Most users will deal with the deprecation when they upgrade.
  • The 3.2 version avoids excessive code generation, but doesn't add a method call to the basic path.
  • The 3.2 + 1 version pretty much generates a 1 line method
  • It will be slightly different for :allow_nil => true, but you get the idea.
@josevalim
Ruby on Rails member

As we have discussed, the deprecation is almost impossible to get it right. If we are going down that road, I would just break backcompat or provide a different method with different API.

@jeremy
Ruby on Rails member

Let's not hijack this pull request by opening another can of worms.

It either stands on its own merits (and benchmarks) or does not.

This other public-only delegation change can be made separately, whether or not this cleanup is applied.

@bogdan, do you care to pursue this further? Convincing benchmarks would include: smaller memory footprint, faster execution, fewer object allocations.

@josevalim
Ruby on Rails member
@jeremy
Ruby on Rails member

@jonleighton that said, I love that approach and would advocate breaking compat at 3.2 for it. Topic for the other PR though.

@jonleighton
Ruby on Rails member

Okay, sorry for hijacking the thread. But if we were going to go down the road of breaking backwards compat for 3.2, then this pull request is redundant, so it seems a bit pointless asking @bogdan to do benchmarks etc if we intend to later change it.

@dasch

Oooookay, so I'm just going to stop working on my API-changing pull request (#2733) until this is sorted out.

@bogdan

With current implementation where allow_nil and not allow_nil implementations are separated I am not sure I can show better memory usage benchmark because the amount of generated code reduced.

All that I wanted to do is move implementation from generated code to reused code and keep it like this no matter how behavior gonna change in future. Because it seems right to do code eval like this. Always.
As I said before: people that meet performance problems with delegate will get rid of it easily.

Anyway I don't have any undoubted reason why this is better. So you can just close this PR.

@bogdan bogdan closed this Dec 21, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment