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

speed up ActiveModel::Dirty#attribute_changed? #24511

Merged
merged 1 commit into from Apr 11, 2016

Conversation

Projects
None yet
5 participants
@lihanli
Contributor

lihanli commented Apr 11, 2016

Summary

This change speeds up ActiveModel::Dirty#attribute_changed? by setting the default parameter for options to nil instead of a hash.

Other Information

Benchmark results

Warming up --------------------------------------
           unchanged     1.000  i/100ms
Calculating -------------------------------------
           unchanged      0.399  (± 0.0%) i/s -      2.000 

Pausing here -- run Ruby again to measure the next benchmark...
Warming up --------------------------------------
            with nil     1.000  i/100ms
Calculating -------------------------------------
            with nil      0.501  (± 0.0%) i/s -      3.000  in   5.984194s

Comparison:
            with nil:        0.5 i/s
           unchanged:        0.4 i/s - 1.26x slower

@lihanli lihanli changed the title from set default parameter to nil to speed up attribute_changed? to speed up ActiveModel::Dirty#attribute_changed? Apr 11, 2016

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 11, 2016

Nice speedup! Please include your benchmark results in the commit message.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 11, 2016

Not sure if there is a speedup here. We are trading an object allocation with a if statement, sometimes that is slow.

I changed the benchmark code to remove the object allocation from the benchmark and the result is almost the same taking in consideration the standard deviation:

require 'bundler/setup'
require 'benchmark/ips'
require 'active_model'

# run this
# bundle exec ruby benchmark.rb && second_run=y bundle exec ruby benchmark.rb

class Person
  include ActiveModel::Dirty

  define_attribute_methods :name, :age

  def initialize(name)
    @name = name
  end

  def name=(val)
    name_will_change! unless val == @name
    @name = val
  end

end

module ActiveModel::Dirty
  def attribute_changed_new?(attr, options = nil)
    result = changes_include?(attr)
    if options
      result &&= options[:to] == __send__(attr) if options.key?(:to)
      result &&= options[:from] == changed_attributes[attr] if options.key?(:from)
    end
    result
  end
end

person = Person.new('tom')
person.name = 'bob'

Benchmark.ips do |x|
  x.report('old code') do
    person.attribute_changed?(:name)
  end

  x.report('new code') do
    person.attribute_changed_new?(:name)
  end
end
Calculating -------------------------------------
            old code    36.611k i/100ms
            new code    69.027k i/100ms
-------------------------------------------------
            old code      1.458M (±19.7%) i/s -      6.956M
            new code      1.743M (±12.9%) i/s -      8.559M
@jeremy

This comment has been minimized.

Member

jeremy commented Apr 11, 2016

@rafaelfranca Seems the savings are the options.key?(…) checks

@rafaelfranca

View changes

activemodel/lib/active_model/dirty.rb Outdated
@@ -174,10 +174,12 @@ def changed_attributes
end
# Handles <tt>*_changed?</tt> for +method_missing+.
def attribute_changed?(attr, options = {}) #:nodoc:
def attribute_changed?(attr, options = nil)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 11, 2016

Member

Could you keep the # :nodoc: comment? This method is private API.

@lihanli lihanli force-pushed the lihanli:activemodel-dirty-attribute-changed branch Apr 11, 2016

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 11, 2016

Seems the savings are the options.key?(…) checks

That makes sense, but even with that I wonder why there is no noticeable difference in my benchmarks.

@lihanli

This comment has been minimized.

Contributor

lihanli commented Apr 11, 2016

@rafaelfranca I ran your benchmark unchanged and got this

Warming up --------------------------------------
            old code    32.176k i/100ms
            new code    34.837k i/100ms
Calculating -------------------------------------
            old code      1.595M (± 3.5%) i/s -      7.947M
            new code      1.942M (± 3.9%) i/s -      9.685M
set default parameter to nil to speed up attribute_changed?
Benchmark results:

Warming up --------------------------------------
            old code    32.176k i/100ms
            new code    34.837k i/100ms
Calculating -------------------------------------
            old code      1.595M (± 3.5%) i/s -      7.947M
            new code      1.942M (± 3.9%) i/s -      9.685M

@lihanli lihanli force-pushed the lihanli:activemodel-dirty-attribute-changed branch to 73e2dbe Apr 11, 2016

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 11, 2016

👍

@lihanli

This comment has been minimized.

Contributor

lihanli commented Apr 11, 2016

@jeremy thanks, I updated the commit message

@jeremy jeremy merged commit 20ffb63 into rails:master Apr 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 11, 2016

❤️ 💚 💙 💛 💜

@bogdan

This comment has been minimized.

Contributor

bogdan commented Apr 11, 2016

Wouldn't modern ruby options parsing in method signature be faster than this one?

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 11, 2016

@bogdan Please do investigate keyword args speed 😁

@bogdan

This comment has been minimized.

Contributor

bogdan commented Apr 11, 2016

@jeremy

around the same performance with current master

https://gist.github.com/36187f53b1cd9d870572509e3c76b682


$ ruby bogdan_change_attributes.rb
Warming up --------------------------------------
           unchanged     1.000  i/100ms
Calculating -------------------------------------
           unchanged      0.379  (± 0.0%) i/s -      2.000  in   5.284255s

Pausing here -- run Ruby again to measure the next benchmark...
Warming up --------------------------------------
            with nil     1.000  i/100ms
Calculating -------------------------------------
            with nil      0.375  (± 0.0%) i/s -      2.000  in   5.335696s

Comparison:
           unchanged:        0.4 i/s
            with nil:        0.4 i/s - 1.01x slower
@jeremy

This comment has been minimized.

Member

jeremy commented Apr 12, 2016

@bogdan Could you compare with @rafaelfranca's script? This one includes object allocation which makes for noisier results.

In any case, this is a good spot to switch to keyword args if it doesn't worsen perf. PR welcome 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment