drapergem / draper

Decorators/View-Models for Rails Applications
MIT License
5.23k stars 527 forks source link

Improve performance of method call via `delegate_all` #911

Closed y-yagi closed 2 months ago

y-yagi commented 2 years ago

Description

Currently, delegatable? call private_methods to check method is private or not. The private_methods generates an Array of methods per call. So a method call via delegate_all generates an extra Array every time.

This patch use private_method_defined? instead of private_methods. This reduce the extra Array.

The inherit argument for private_method_defined? is supported since Ruby 2.6. So this patch only works for >= Ruby 2.6. Rubies old than Ruby 2.6 keep using private_methods.

Ref: https://bugs.ruby-lang.org/issues/14944

Benchmark is here.


require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "activerecord"
  gem "sqlite3"
  if ENV["USE_FORKED_GEM"]
    gem "draper", github: "y-yagi/draper", branch: "improve-delegate_all-performance"
  else
    gem "draper"
  end
  gem "benchmark-ips"
end

require "active_record"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.string :name, null: false
    t.timestamps
  end
end

ActiveRecord::Base.logger = Logger.new(STDOUT)

class User < ActiveRecord::Base
end

class UserDecorator < Draper::Decorator
  delegate_all

  def decorated_name
    "'#{name}'"
  end
end

User.create!(name: "Dummy User")
u = UserDecorator.decorate(User.first)

Benchmark.ips do |x|
  x.report(ENV["USE_FORKED_GEM"] == "true" ? "forked draper" : "released draper") do
    1000.times { u.decorated_name }
  end

  x.save! ENV["SAVE_FILE"] if ENV["SAVE_FILE"]
  x.compare!
end
$ SAVE_FILE=result.out ruby draper.rb
$ USE_FORKED_GEM=true SAVE_FILE=result.out ruby draper.rb
...
Comparison:
       forked draper:      454.5 i/s
     released draper:       63.8 i/s - 7.12x  (± 0.00) slower

Testing

Existing tests covers this change.

y-yagi commented 2 years ago

Failed test is unrelated with this change. That test will fix by #912.

y-yagi commented 2 years ago

Rebased with master and now CI passes.