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

Avoid generating full changes hash on every save #32497

Merged
merged 1 commit into from Apr 9, 2018

Conversation

Projects
None yet
3 participants
@eugeneius
Member

eugeneius commented Apr 8, 2018

changed_attribute_names_to_save is called in keys_for_partial_write, which is called on every save when partial writes are enabled.

We can avoid generating the full changes hash by asking the mutation tracker for just the names of the changed attributes. At minimum this saves one array allocation per attribute, but will also avoid calling Attribute#original_value which is expensive for serialized attributes.

r? @sgrif

Avoid generating full changes hash on every save
`changed_attribute_names_to_save` is called in `keys_for_partial_write`,
which is called on every save when partial writes are enabled.

We can avoid generating the full changes hash by asking the mutation
tracker for just the names of the changed attributes. At minimum this
saves one array allocation per attribute, but will also avoid calling
`Attribute#original_value` which is expensive for serialized attributes.
@eugeneius

This comment has been minimized.

Member

eugeneius commented Apr 8, 2018

Benchmarks:

Simple case: model with no attributes
begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", path: "."
  gem "sqlite3"
  gem "benchmark-ips"
end

require "active_record"

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

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end
end

class Post < ActiveRecord::Base
end

post = Post.new

Benchmark.ips do |x|
  x.report("mutations_from_database.changed_attribute_names") { post.changed_attribute_names_to_save }
  x.report("changes_to_save.keys") { post.changes_to_save.keys }
end
Warming up --------------------------------------
mutations_from_database.changed_attribute_names
                        39.860k i/100ms
changes_to_save.keys    25.917k i/100ms
Calculating -------------------------------------
mutations_from_database.changed_attribute_names
                        477.041k (± 3.6%) i/s -      2.392M in   5.019760s
changes_to_save.keys    286.142k (± 4.2%) i/s -      1.451M in   5.080372s
Complex case: model with a large serialized attribute
begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", path: "."
  gem "sqlite3"
  gem "benchmark-ips"
end

require "active_record"

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

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :content
  end
end

class Post < ActiveRecord::Base
  serialize :content
end

post = Post.new(content: [{ foo: "bar" }] * 100)

Benchmark.ips do |x|
  x.report("mutations_from_database.changed_attribute_names") { post.changed_attribute_names_to_save }
  x.report("changes_to_save.keys") { post.changes_to_save.keys }
end
Warming up --------------------------------------
mutations_from_database.changed_attribute_names
                        21.255k i/100ms
changes_to_save.keys     3.398k i/100ms
Calculating -------------------------------------
mutations_from_database.changed_attribute_names
                        231.289k (± 3.4%) i/s -      1.169M in   5.060643s
changes_to_save.keys     35.805k (± 2.3%) i/s -    180.094k in   5.032551s
@kamipo

This comment has been minimized.

Member

kamipo commented Apr 9, 2018

This improvement was accidentally lost in the revert commit 5fcbdcf.

@kamipo kamipo merged commit bc9fb9c into rails:master Apr 9, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Apr 9, 2018

Merge pull request #32497 from eugeneius/mutation_tracker_changed_att…
…ribute_names

Avoid generating full changes hash on every save

kamipo added a commit that referenced this pull request Apr 10, 2018

Merge pull request #32497 from eugeneius/mutation_tracker_changed_att…
…ribute_names

Avoid generating full changes hash on every save
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment