Refactor ActionDispatch::Http::UploadedFile #2711

Merged
merged 1 commit into from Aug 28, 2011

Conversation

Projects
None yet
4 participants
Contributor

dasch commented Aug 27, 2011

Replace the methods with a single call to delegate.

Member

josevalim commented Aug 27, 2011

Owner

tenderlove commented Aug 27, 2011

I'd rather we don't regress in performance, but at the same time I'm tired of getting pull requests to convert this to using delegate. :-(

require 'benchmark'
require 'active_support/core_ext/module/delegation'

class Target
  def hello; 'world'; end
end

class MethodDelegate
  def initialize
    @target = Target.new
  end

  def hello; @target.hello; end
end

class MetaDelegate
  delegate :hello, :to => :@target

  def initialize
    @target = Target.new
  end
end

n      = 10_000_000
method = MethodDelegate.new
meta   = MetaDelegate.new

Benchmark.bm(7) do |x|
  x.report('method') {
    n.times { method.hello }
  }
  x.report('meta') {
    n.times { meta.hello }
  }
end
[aaron@mobile-166-187-073-158 activesupport (master)]$ ruby -I lib del.rb
              user     system      total        real
method    5.570000   0.010000   5.580000 (  5.778747)
meta      7.340000   0.020000   7.360000 (  7.402884)
[aaron@mobile-166-187-073-158 activesupport (master)]$ 
Contributor

dasch commented Aug 27, 2011

@tenderlove: is this due to runtimes inlining the normal case, but not the metaprogramming one? In that case, would there be a way to implement delegate such that they can? It's rather unfortunate that we need to accept verbosity because of this fact.

Owner

tenderlove commented Aug 27, 2011

There are a few performance issues going on. It's possible the runtime could inline the methods (but not likely in MRI). The main issues are:

  • Stack depth impacts GC time
  • paying an extra method call __send__
  • *args contraction (we must build an array that is just GC'd)
  • Splatting the args back to the __send__

In this case, since we know the arity for most of the methods to be 0, we could just use a regular class_eval and metaprogram them. It would shorten the code and be about the same speed as regular method definition:

require 'benchmark'
require 'active_support/core_ext/module/delegation'

class Target
  def hello; 'world'; end
end

class TargetWrapper
  def initialize
    @target = Target.new
  end
end

class MethodDelegate < TargetWrapper
  def hello; @target.hello; end
end

class MetaDelegate < TargetWrapper
  delegate :hello, :to => :@target
end

class EvalDelegate < TargetWrapper
  [ :hello ].each do |method|
    class_eval "def #{method}; @target.#{method}; end"
  end
end

n      = 10_000_000
method = MethodDelegate.new
meta   = MetaDelegate.new
ev     = EvalDelegate.new

Benchmark.bm(7) do |x|
  x.report('method') { n.times { method.hello } }
  x.report('meta')   { n.times { meta.hello } }
  x.report('eval')   { n.times { ev.hello } }
end
[aaron@mobile-166-187-073-158 activesupport (master)]$ ruby -I lib del.rb
              user     system      total        real
method    5.250000   0.000000   5.250000 (  5.281733)
meta      7.230000   0.010000   7.240000 (  7.248358)
eval      5.590000   0.000000   5.590000 (  5.614715)
[aaron@mobile-166-187-073-158 activesupport (master)]$

I'd prefer we go the class_eval route. :-)

Contributor

dasch commented Aug 27, 2011

@tenderlove: I can see that the implementation of delegate uses __send__ in the module_eval string - why is this necessary? Do we want people to delegate to private methods? And wouldn't it be faster to simply do:

module_eval(<<-EOS, file, line - 1)
  def #{method_prefix}#{method}(*args, &block)
    #{to}.#{method}(*args, &block)
  end
EOS

This doesn't solve the problem with *args, but at least the stack depth would be the same, and we wouldn't need to call __send__.

Owner

tenderlove commented Aug 27, 2011

Ya, current delegate behavior is that it will expose private methods, and we shouldn't break it. @jonleighton proposed we change delegate to not expose private methods, but I think it's a different discussion.

I'm also more in favor of just using class_eval in this case since it doesn't depend on AS. If the arity is known, metaprogramming delegate methods with class_eval is so simple. I'm not sure why we need an API around it.

Anyway, if you change this patch to use class_eval, I'll merge it in.

Contributor

dasch commented Aug 27, 2011

Sweet, I'll look at it tomorrow or Monday.

On 27/08/2011, at 21.40, tenderlovereply@reply.github.com wrote:

Ya, current delegate behavior is that it will expose private methods, and we shouldn't break it. @jonleighton proposed we change delegate to not expose private methods, but I think it's a different discussion.

I'm also more in favor of just using class_eval in this case since it doesn't depend on AS. If the arity is known, metaprogramming delegate methods with class_eval is so simple. I'm not sure why we need an API around it.

Anyway, if you change this patch to use class_eval, I'll merge it in.

Reply to this email directly or view it on GitHub:
#2711 (comment)

Contributor

dasch commented Aug 28, 2011

@tenderlove: I've changed the code per your comments, using class_eval.

Would you be open to discussing a change to delegate on the mailing list?

Owner

tenderlove commented Aug 28, 2011

Great! I will apply.

Yes, I'm open to discuss anything on the ML! Including delegate. :-)

tenderlove merged commit e145acd into rails:master Aug 28, 2011

Contributor

dasch commented Aug 29, 2011

@tenderlove: I've tried changing it, and it became apparent why __send__ is needed -- delegating writer methods would be impossible otherwise, as something=(*args, &block) is not syntactically valid. It's unfortunate, though, as delegate is a nice declarative API.

Contributor

dasch commented Aug 29, 2011

Actually, with a special case for writer methods it could work.

Member

jonleighton commented Aug 29, 2011

@dasch I tried to change this recently, and subsequently reverted in 8ba491a. See discussion in aa1d1e4.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment