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

Allocation free Integer#to_s #27736

Merged

Conversation

@casperisfine
Copy link

@casperisfine casperisfine commented Jan 19, 2017

We were profiling in production, and were surprised to see NumericWithFormat#to_s account for 2% of global object allocations.

       34   (2.0%)          34   (2.0%)     ActiveSupport::NumericWithFormat#to_s

Looking at the code, it turns out that even if you want to call the regular Integer#to_s (which is fairly common, especially if you display numbers in your views) you end up allocating both an Array (splat args) and a Hash (options).

This PR prevent those allocations if you are not trying to call any advanced formatting.

I know 2 objects isn't much at all, and that this version is a bit more complex to grasp, but for such a hotspot I believe it's worth it.

@rafaelfranca @csfrancis @camilo

@rails-bot
Copy link

@rails-bot rails-bot commented Jan 19, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

I profiled a small benchmark:

StackProf.run(mode: :object, out: '/tmp/stackprof-object-int-to_s-as-fix.dump') do
  1_000.times do
    42.to_s
  end
end

Pure ruby:

==================================
  Mode: object(1)
  Samples: 1000 (0.00% miss rate)
  GC: 0 (0.00%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      1000 (100.0%)        1000 (100.0%)     block (2 levels) in <main>
      1000 (100.0%)           0   (0.0%)     block in <main>
      1000 (100.0%)           0   (0.0%)     <main>
      1000 (100.0%)           0   (0.0%)     <main>

current active_support:

==================================
  Mode: object(1)
  Samples: 3001 (0.00% miss rate)
  GC: 0 (0.00%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      2001  (66.7%)        2001  (66.7%)     ActiveSupport::NumericWithFormat#to_s
      3001 (100.0%)        1000  (33.3%)     block (2 levels) in <main>
      3001 (100.0%)           0   (0.0%)     block in <main>
      3001 (100.0%)           0   (0.0%)     <main>
      3001 (100.0%)           0   (0.0%)     <main>

With this fix:

==================================
  Mode: object(1)
  Samples: 2000 (0.00% miss rate)
  GC: 0 (0.00%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      2000 (100.0%)        1000  (50.0%)     block (2 levels) in <main>
      1000  (50.0%)        1000  (50.0%)     ActiveSupport::NumericWithFormat#to_s
      2000 (100.0%)           0   (0.0%)     block in <main>
      2000 (100.0%)           0   (0.0%)     <main>
      2000 (100.0%)           0   (0.0%)     <main>

So it look like there is still an allocation I haven't removed.

@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

Turns out * even if not assigned to any name, still allocates an array.

So I removed it:

==================================
  Mode: object(1)
  Samples: 1000 (0.00% miss rate)
  GC: 0 (0.00%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      1000 (100.0%)        1000 (100.0%)     ActiveSupport::NumericWithFormat#to_s
      1000 (100.0%)           0   (0.0%)     block (2 levels) in <main>
      1000 (100.0%)           0   (0.0%)     block in <main>
      1000 (100.0%)           0   (0.0%)     <main>
      1000 (100.0%)           0   (0.0%)     <main>

It might break backwards compatibility, in case people used that module on custom classes. But I think it's fair to assume that module is a private API.

@casperisfine casperisfine force-pushed the Shopify:reduce-numeric-with-format-allocations branch Jan 19, 2017
activesupport/lib/active_support/core_ext/numeric/conversions.rb Outdated
private_constant :DEFAULT
def to_s(format = DEFAULT, options = DEFAULT)
return super() if format == DEFAULT
return super(format) if format.is_a?(Integer)

This comment has been minimized.

@matthewd

matthewd Jan 19, 2017
Member

These can both be when clauses, I think?

This comment has been minimized.

@casperisfine

casperisfine Jan 19, 2017
Author

Good point.

activesupport/lib/active_support/core_ext/numeric/conversions.rb Outdated
format, options = args
options ||= {}
DEFAULT = Object.new
private_constant :DEFAULT

This comment has been minimized.

@matthewd

matthewd Jan 19, 2017
Member

It's private, but we're still ending up adding this to core classes. Maybe worth storing it elsewhere?

This comment has been minimized.

@casperisfine

casperisfine Jan 19, 2017
Author

Any idea where it could go?

@casperisfine casperisfine force-pushed the Shopify:reduce-numeric-with-format-allocations branch Jan 19, 2017
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jan 19, 2017

Does this not work?

  def to_s(format = nil, options = nil)
    case format
    when nil
      super()
    when Integer, String
      super(format)
    when :phone
      return ActiveSupport::NumberHelper.number_to_phone(self, options || {})
    when :currency
      return ActiveSupport::NumberHelper.number_to_currency(self, options || {})
    when :percentage
      return ActiveSupport::NumberHelper.number_to_percentage(self, options || {})
    when :delimited
      return ActiveSupport::NumberHelper.number_to_delimited(self, options || {})
    when :rounded
      return ActiveSupport::NumberHelper.number_to_rounded(self, options || {})
    when :human
      return ActiveSupport::NumberHelper.number_to_human(self, options || {})
    when :human_size
      return ActiveSupport::NumberHelper.number_to_human_size(self, options || {})
    else
      super()
    end
  end

When format is nil we rely on the default behavior of the super method, when it's an integer then it's probably Integer#to_s and when it's a string then it's BigDecimal#to_s. The else clause could possibly raise an ArgumentError since it would be unexpected for it to reach that clause.

@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

Does this not work?

Definitely simpler. I stupidly went with DEFAULT = Object.new because I overlooked that if you were to pass option = nil it would be assigned a new hash.

I'll update the PR.

@casperisfine casperisfine force-pushed the Shopify:reduce-numeric-with-format-allocations branch Jan 19, 2017
@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

Updated.

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jan 19, 2017

Not happy with that final super() - we're masking errors like this:

>> 10.to_s(Object.new)
TypeError: no implicit conversion of Object into Integer
	from (irb):1:in `to_s'
	from (irb):1
	from /Users/andyw/.rvm/rubies/ruby-2.4.0/bin/irb:11:in `<main>'

vs.

>> 10.to_s(Object.new)
=> "10"

However a bare super won't work since that'll blow up on the method arity. Maybe it should be super(format) since if it was nil it would've been caught by the previous clause on nil.

@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

Agreed.

@casperisfine casperisfine force-pushed the Shopify:reduce-numeric-with-format-allocations branch to b9bda7f Jan 19, 2017
@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

@pixeltrix updated.

Note that I had to add a case for Symbol, because if you pass a symbol that is not part of the supported list, it will default to the original behavior.

Another slightly backward incompatible change is that before 1.to_s(16, foo: :bar) would have raised and argument error, and it will now silently ignore the options (that was why I was using the DEFAULT const to detect if the argument was passed or not).

return ActiveSupport::NumberHelper.number_to_human_size(self, options)
return ActiveSupport::NumberHelper.number_to_human_size(self, options || {})
when Symbol
super()

This comment has been minimized.

@pixeltrix

pixeltrix Jan 19, 2017
Member

I think this clause is still wrong - we should either delete it and it'll raise the appropriate error (TypeError from Integer or ArgumentError from Float), or we should raise an ArgumentError with an unknown format message rather than just silently returning the default. That would be a better developer experience since it'd blow up if there was a typo in the format name.

This comment has been minimized.

@matthewd

matthewd Jan 19, 2017
Member

I agree.. but IMO we should probably keep that change away from this perf one

This comment has been minimized.

@casperisfine

casperisfine Jan 19, 2017
Author

Yep I agree as well.

This comment has been minimized.

@pixeltrix

pixeltrix Jan 19, 2017
Member

👍 performance change can be backported to 5-0-stable but change to raising can't.

@pixeltrix pixeltrix merged commit 9a1f4bc into rails:master Jan 19, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jan 19, 2017

Backported in 051288d

@casperisfine thanks! 👍

@casperisfine
Copy link
Author

@casperisfine casperisfine commented Jan 19, 2017

Thanks to you!

@casperisfine casperisfine deleted the Shopify:reduce-numeric-with-format-allocations branch Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.