-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Lesser '.' objects for number helpers #24502
Lesser '.' objects for number helpers #24502
Conversation
…every iteration of calling the helper. Eases on some memory bloat
r? @kaspth (@rails-bot has picked a reviewer for you, use r? to override) |
What about to move PS: I have spotted another strings initializations used in those two files. Aren't they causing the same problem? |
@@ -30,7 +30,7 @@ def convert | |||
formatted_string = | |||
if BigDecimal === rounded_number && rounded_number.finite? | |||
s = rounded_number.to_s('F') + '0'*precision | |||
a, b = s.split('.', 2) | |||
a, b = s.split('.'.freeze, 2) | |||
a + '.' + b[0, precision] |
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.
We could further reduce allocations by appending to the "a" string here using << instead of +. We can do that because split returns a new string. I think we can use the same trick above where we are adding the zeroes to generate "s".
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.
Yeah, i actually missed it. It was thr in the report too line 37 a + '.' + b[0, precision]
. This is also a problem and overall we have 3000 dot strings
3002 "."
1000 /Users/agupta/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.6/lib/active_support/number_helper/number_to_delimited_converter.rb:15
1000 /Users/agupta/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.6/lib/active_support/number_helper/number_to_rounded_converter.rb:33
1000 /Users/agupta/.rvm/gems/ruby-2.2.2/gems/activesupport-4.2.6/lib/active_support/number_helper/number_to_rounded_converter.rb:37
From the couple of approaches we have
require 'rubygems'
require 'benchmark/ips'
# Current Implementation
def method_one
a = 'foo'
b = 'bar'
a + '.' + b
end
# Appending
def method_two
a = 'foo'
b = 'bar'
a << '.' << b
end
# Interpolation
def method_three
a = 'foo'
b = 'bar'
a << ".#{b}"
end
Benchmark.ips do |x|
x.config(:time => 5, :warmup => 2)
x.report("method_one") { method_one }
x.report("method_two") { method_two }
x.report("method_three") { method_three }
x.compare!
end
p "method_one output: #{method_one}"
p "method_two output: #{method_two}"
p "method_three output: #{method_three}"
Benchmark Output
agupta01sjl:check_active_support agupta$ ruby bm.rb
Warming up --------------------------------------
method_one 96.895k i/100ms
method_two 103.509k i/100ms
method_three 99.381k i/100ms
Calculating -------------------------------------
method_one 2.290M (± 4.5%) i/s - 11.434M
method_two 2.470M (± 6.0%) i/s - 12.318M
method_three 2.487M (± 5.3%) i/s - 12.423M
Comparison:
method_three: 2486823.3 i/s
method_two: 2469837.1 i/s - same-ish: difference falls within error
method_one: 2290160.7 i/s - same-ish: difference falls within error
"method_one output: foo.bar"
"method_two output: foo.bar"
"method_three output: foo.bar"
method_three (which is interpolation) is better but it would also create a string like .100
in this case. I am planning to go ahead with method_two
a << '.'.freeze << b[0, precision]
Wdyt @schneems ? The above removes all the extra 3000 strings and single allocation.
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.
Weird, this gives you an error
'.'.freeze << "what"
RuntimeError: can't modify frozen String
I'm surprised that putting another string in front doesn't. I guess this is because a << '.'.freeze
gets evaluated first and returns a non-frozen string.
I would have done the more verbose
a << '.'.freeze
a << b[0, precision]
Your way is fine.
Since "s" is also not an input string and it doesn't look like it's used anywhere else we could do the same there
s = rounded_number.to_s('F'.freeze)
s << '0'.freeze * precision
That would save us two string allocations including getting rid of even more "0"
strings.
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.
Please don't put frozen strings in constants, it is slower. Thanks for the work here! Very thorough PR. |
r? @schneems |
@simi Shared won't help us much as all the helpers are not using |
…ng to do the same string manipulation. This was we avoid the duplicate strings with freeze and append modifies existing string
SummarizingOn the basis of above example which was iterating n times number_to_delimited We saved n extra '.' string initializations 1000 extra strings shaved for above example. number_to_rounded We saved n*3 extra '.' string initializations 4000 extra strings shaved for above example. |
Thanks for all the work! |
Summary
ActiveSupport::NumberHelper
number_to_delimited
andnumber_to_rounded
methods were creating duplicate dot ('.'
) strings in my project. When used with a iterating loop lot of string allocation happens. Debugging done via derailed_benchmarksHere is a simplified example:
number_to_delimited
Notice 1000 dot initializations in below report
number_to_rounded
Since number_to_rounded internally calls
NumberToDelimitedConverter
converter the effect is more.Notice 1000 - 1000 dot initializations in below report for number_to_delimited_converter & number_to_rounded_converter
After freezing the duplicate string were not formed. cc @schneems