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 #930

Closed

Conversation

y-yagi
Copy link
Collaborator

@y-yagi y-yagi commented 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.

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

Description

Detail your changes here.
A few sentences describing the overall goals of the pull request's commits will suffice.
Some questions you might answer:

  • Why was this change required?
  • Did you have any tough decisions to make? Which one(s) did you go with and why?
  • Are there any deployment impacts to this change?
  • Is there something you aren't happy with or that needs extra attention?

Testing

Outline steps to test your changes.

  1. Go here.
  2. Click this.
  3. See that.

To-Dos

  • tests
  • documentation

References

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 closed this Aug 21, 2024
@y-yagi y-yagi deleted the improve-delegate_all-performance-retry branch August 21, 2024 06:02
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