Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of method call via delegate_all #911

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Dec 14, 2021

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 y-yagi force-pushed the improve-delegate_all-performance branch from 0ed9c96 to adb4fc8 Compare December 14, 2021 01:57
@y-yagi
Copy link
Contributor Author

y-yagi commented Dec 14, 2021

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

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.

```ruby

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

```bash
$ 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
```
@y-yagi y-yagi force-pushed the improve-delegate_all-performance branch from adb4fc8 to 5a03248 Compare December 16, 2021 23:31
@y-yagi
Copy link
Contributor Author

y-yagi commented Dec 16, 2021

Rebased with master and now CI passes.

y-yagi added a commit to ClinicalPlatform/draper that referenced this pull request Dec 20, 2021
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.

```ruby

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

```bash
$ 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
```

This is a fork of drapergem#911
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant