-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
PERF: Recover ActiveRecord::pluck
performance.
#30524
Conversation
r? @sgrif |
raise ArgumentError, "unknown relation value #{name.inspect}" | ||
end | ||
DEFAULT_VALUES.fetch(name) | ||
rescue KeyError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a rescue
, can we do the raise in a block passed to fetch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very internal.. do we care enough to produce our own exception, or can we just leave it up to a blockless fetch
? It'd change the exception raised, but given this is a "something is broken inside Rails" error, not "something in your app is wrong", that doesn't seem important.
If so, should we then inline this private method into its single caller? default_value_for(name)
doesn't seem much more descriptive than DEFAULT_VALUES.fetch(name)
.
Looks like this might be a common hotspot for other queries as well: Benchmark Script
Rails 4.2.9
Rails 5.1.3
Rails Master
Rails Master with patch
Compared to 4.2.9, Rails master with patch is still about 8% slower. |
```ruby require 'active_record' require 'benchmark/ips' ActiveRecord::Base.establish_connection(ENV.fetch('DATABASE_URL')) ActiveRecord::Migration.verbose = false ActiveRecord::Schema.define do create_table :users, force: true do |t| t.string :name, :email t.timestamps null: false end end attributes = { name: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.', email: 'foobar@email.com' } class User < ActiveRecord::Base; end 1000.times do User.create!(attributes) end Benchmark.ips do |x| x.config(time: 10, warmup: 2) x.report('pluck 1 column') do User.pluck(:id) end x.report('pluck 2 columns') do User.pluck(:id, :email) end x.report('pluck 1 column with scope') do User.where(id: 1000).pluck(:id) end x.report('pluck 2 columns with scope') do User.where(id: 1000).pluck(:id, :email) end end ``` ``` Calculating ------------------------------------- pluck 1 column 122.000 i/100ms pluck 2 columns 74.000 i/100ms pluck 1 column with scope 615.000 i/100ms pluck 2 columns with scope 515.000 i/100ms ------------------------------------------------- pluck 1 column 1.272k (± 3.9%) i/s - 12.810k pluck 2 columns 750.096 (± 3.3%) i/s - 7.548k pluck 1 column with scope 6.074k (± 4.1%) i/s - 60.885k pluck 2 columns with scope 5.158k (± 2.7%) i/s - 52.015k ``` ``` Calculating ------------------------------------- pluck 1 column 126.000 i/100ms pluck 2 columns 78.000 i/100ms pluck 1 column with scope 457.000 i/100ms pluck 2 columns with scope 434.000 i/100ms ------------------------------------------------- pluck 1 column 1.266k (± 2.1%) i/s - 12.726k pluck 2 columns 795.061 (± 3.0%) i/s - 7.956k pluck 1 column with scope 4.660k (± 2.1%) i/s - 46.614k pluck 2 columns with scope 4.355k (± 2.3%) i/s - 43.834k ``` ``` Calculating ------------------------------------- pluck 1 column 126.000 i/100ms pluck 2 columns 78.000 i/100ms pluck 1 column with scope 539.000 i/100ms pluck 2 columns with scope 481.000 i/100ms ------------------------------------------------- pluck 1 column 1.308k (± 3.4%) i/s - 13.104k pluck 2 columns 798.604 (± 2.8%) i/s - 8.034k pluck 1 column with scope 5.530k (± 3.4%) i/s - 55.517k pluck 2 columns with scope 4.914k (± 2.7%) i/s - 49.543k ``` ``` Calculating ------------------------------------- pluck 1 column 139.000 i/100ms pluck 2 columns 79.000 i/100ms pluck 1 column with scope 580.000 i/100ms pluck 2 columns with scope 526.000 i/100ms ------------------------------------------------- pluck 1 column 1.337k (± 3.0%) i/s - 13.483k pluck 2 columns 806.776 (± 2.7%) i/s - 8.137k pluck 1 column with scope 5.924k (± 4.1%) i/s - 59.160k pluck 2 columns with scope 5.276k (± 3.1%) i/s - 53.126k ```
3b0a615
to
3381278
Compare
@sgrif Thanks for reviewing. Could you let me know if you're getting the same benchmark results locally? |
I haven't had a chance to run anything locally. I trust that I am. I'm off this week, as I'm in the middle of moving to a new country. This patch looks fine. I agree with Matthew's comment, but we can fix that later. |
@sgrif Thanks for reviewing! :) |
The code is not original one, the Relation::SINGLE_VALUE_METHODS values are nil.
Summary
I'm seeing a performance regression when using pluck with a scope. One thing I noticed when profiling with
ruby-prof
is thatSymbol#===
was taking quite abit of time.Benchmark script used
Rails 4.2.9
Rails 5.1.3
Rails Master
Rails Master with Patch