Comparing a BigDecimal to true/false on write_attribute is slow #8673

Closed
mathie opened this Issue Jan 1, 2013 · 13 comments

Comments

Projects
None yet
5 participants

mathie commented Jan 1, 2013

Hi,

While doing a spot of profiling on an app this afternoon (what else does one do on New Year's Day, eh?), I noticed a major hot spot was BigDecimal#== and, in particular, when it's invoked from ActiveRecord::AttributeMethods::Write#convert_number_column_value, so I thought I'd dig into it a bit.

Some simple non-Rails-specific code to allow me to profile BigDecimal#==:

require 'bigdecimal'

1_000_000.times do
  BigDecimal('3') == true
end

That script takes ~7s to run on my Mac. Compared with:

require 'bigdecimal'

1_000_000.times do
  BigDecimal('3') == 0
end

which takes about 1.2s.

I'm struggling to trace the code in BigDecimal#== to see exactly what's happening, but the profile seems to suggest it's generating, then catching, a NameError exception:

Screen Shot 2013-01-01 at 14 02 33

That seems suboptimal. This pretty much seems to be a bug in BigDecimal, which I'll report upstream.

However, it's causing ActiveRecord models which use BigDecimal to be unnecessarily slow, as every write_attribute call with a BigDecimal value causes it to be compared to true, then compared to false (see write.rb#57). In the app I've been profiling, I've monkey patched the method to be:

ActiveRecord::AttributeMethods::Write.module_eval do
  def convert_number_column_value(value)
    if value.is_a?(Numeric)
      value
    elsif value == false
      0
    elsif value == true
      1
    elsif value.is_a?(String) && value.blank?
      nil
    else
      value
    end
  end
end

This has reduced my profiling test case from 50 seconds to 20 seconds, so it seems to be doing something right. What do you reckon?

mathie commented Jan 1, 2013

I've now reported this to the ruby tracker, too: Bug #7645.

Contributor

al2o3cr commented Jan 1, 2013

What's going on here is very tricky - there's a NameError alright, but it's because a method is missing on TrueClass.

To demonstrate, try this in a Rails 3.x console (handy since BasicObject stays out of our way):

irb: class Wat < BasicObject; def method_missing(method, *args); ::Kernel.puts(method); ::Kernel.puts(args.inspect); end; end
===> nil
irb: BigDecimal('3') == Wat.new
coerce
[#<BigDecimal:1034b7c70,'0.3E1',9(18)>]
===> false
irb: true.coerce(BigDecimal('3'))
NoMethodError: undefined method `coerce' for true:TrueClass

Looks like the exception happens in do_coerce:

https://github.com/ruby/ruby/blob/trunk/numeric.c#L234

which is in turn called by rb_num_coerce_cmp:

https://github.com/ruby/ruby/blob/trunk/numeric.c#L264

rb_num_coerce_cmp is specifically requesting that no exception be emitted by passing in FALSE in the last parameter; looks like the code in do_coerce is much like rescue nil.

The gotcha is that the code still generates the exception before dropping it on the floor, thus soaking up execution time.

I've run across a similar issue doing this in deeply nested functions:

foo, bar = baz(wat)

if baz returns nil, Ruby tries to convert the nil into an array using to_ary - and silently swallows the exception if the method isn't defined. Of course, with a deep stack, calling caller as part of the NoMethodError initializer isn't free.

Contributor

al2o3cr commented Jan 1, 2013

BTW, reversing the comparison (so true == BigDecimal('3')) makes the whole thing run FAR faster. Makes sense, since the call is now to BigDecimal#coerce, which does actually exist.

eregon commented Jan 1, 2013

@al2o3cr You're right, I commented on the ruby tracker.

The comparison should be done in the other sense, indeed: true == value, that would call TrueClass#==, which does nothing fancy. (BigDecimal#coerce being never called)

Interesting catch @mathie. Have you tried profiling changing the code to use case instead? I think something like this should work the same:

case value
when FalseClass
  0
when TrueClass
  1
when String
  value.presence
else
  value
end

Hope that gets fixed in Ruby, but if we can make something to avoid the coercion without bringing any other issue, I'm 👍.

mathie commented Jan 7, 2013

@carlosantoniosasilva The case statement looks a lot more elegant and avoids the coercion. However, it changes the return value when it's a string. Before it would be the string value if true, and nil if false. In your proposed version, it's true/false. The truthiness of the result is maintained, but I can't help feeling /somebody/ relies on the string being returned. :)

How about:

case value
when FalseClass
  0
when TrueClass
  1
when String
  value.present? ? value : nil
else
  value
end

Bit more long winded but retains the existing return value semantics exactly.

The benchmark looks compelling to me.

Thanks. I may be missing something about the return value though, isn't value.presence the same as value.present? ? value : nil? Checking the implementation:

def presence
  self if present?
end

and testing:

>> value = 'foo'
=> "foo"
>> value.presence
=> "foo"
>> value.present? ? value : nil
=> "foo"
>> value = nil
=> nil
>> value.presence
=> nil
>> value.present? ? value : nil
=> nil

mathie commented Jan 7, 2013

Nah, I'm missing something; I never knew that, and misread it as present?. Cool.

In which case, your case statement gets a 👍 from me.

Nice, thanks.

carlosantoniodasilva added a commit that referenced this issue Jan 7, 2013

Refactor write attribute logic to convert number column value
This is an improvement for issue #8673:
    "Comparing a BigDecimal to true/false on write_attribute is slow"

It seems to be an issue with Ruby itself, related to the "coerce" method
being called in TrueClass/FalseClass due to the == condition, triggering
method_missing, then raising a NameError that's later catched.

This issue was also opened in Ruby tracker:
    https://bugs.ruby-lang.org/issues/7645.

This refactoring avoid the coerce call by using a case statement, which
gives us better readability as well. A simple benchmark:

----------

require 'benchmark/ips'
require 'bigdecimal'

Benchmark.ips do |x|
  x.report("== true")   { BigDecimal('3') == true }
  x.report("TrueClass") { TrueClass === BigDecimal('3') }
  x.report("== 0")      { BigDecimal('3') == 0 }
  x.report("Numeric")   { Numeric === BigDecimal('3') }
end

Calculating -------------------------------------
             == true      6427 i/100ms
           TrueClass     47297 i/100ms
                == 0     35923 i/100ms
             Numeric     55530 i/100ms
-------------------------------------------------
             == true    75878.5 (±21.6%) i/s -     359912 in   5.004392s
           TrueClass  1249547.0 (±13.1%) i/s -    6148610 in   5.035964s
                == 0   666856.3 (±13.3%) i/s -    3268993 in   5.013789s
             Numeric  1269300.9 (±11.3%) i/s -    6274890 in   5.028458s

----------

Master has a very different implementation, and there are apparently no
similar conversions at this point, it's mainly delegated to the column
type cast, but I'll check if something needs to be changed there as well.

Closes #8673.

@mathie done, hope that helps improving it, now lets wait the Ruby issue :). Thanks!

Contributor

mrkn commented Jan 10, 2013

I've fixed Bug #7645 now on trunk.
So ruby 2.0 won't have this issue.

@mrkn thank you!

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