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

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

y-yagi
Copy link
Collaborator

@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
Collaborator Author

y-yagi commented Dec 14, 2021

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

@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
Collaborator 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
@y-yagi y-yagi closed this Aug 21, 2024
@y-yagi y-yagi reopened this Aug 21, 2024
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 5a03248 to 2d53439 Compare August 25, 2024 02:09
@Alexander-Senko Alexander-Senko added this to the 4.0.3 milestone Aug 31, 2024
@y-yagi y-yagi merged commit 7093384 into drapergem:master Sep 4, 2024
8 checks passed
@y-yagi y-yagi deleted the improve-delegate_all-performance branch September 4, 2024 05:44
Alexander-Senko added a commit that referenced this pull request Oct 4, 2024
Added support for latest Ruby (upto 3.4) and Rails (upto 8) versions.

## Fixes

* `CollectionDecorator#respond_to?` for non-ORM collections
  (#920)
* Using Draper outside of controller scope
  (#927)
* Decoration of AR associations
  (#932)

## Performance

* Sped up delegation via `delegate_all`
  (#911)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants