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

Interpolate '' instead of nil when multiple is false. #32302

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

dillonwelch
Copy link
Contributor

Summary

"my string #{nil}" results in an additional '' string allocation, I'm
guessing because the nil has to be converted to a string.

"my string #{'[]' if multiple}" results in "my string #{nil}" if
multiple is false. Doing "my string #{''}" does not result in an extra
string allocation. I moved the if multiple logic into a method so I only
had to make the change once.

Other Information

Benchmarking:

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 "benchmark-ips"
  gem "rails"
end

def allocate_count
  GC.disable
  before = ObjectSpace.count_objects
  yield
  after = ObjectSpace.count_objects
  after.each { |k,v| after[k] = v - before[k] }
  after[:T_HASH] -= 1 # probe effect - we created the before hash.
  GC.enable
  result = after.reject { |k,v| v == 0 }
  GC.start
  result
end

@html_options = {}

def master_version(multiple=nil)
  "hi#{"[]" if multiple}"
end

def fast_version(multiple=nil)
  str = multiple ? "[]" : ''
  "hi#{str}"
end

def test
  puts "master_version"
  puts allocate_count { 1000.times { master_version } }
  puts "master_version with arg"
  puts allocate_count { 1000.times { master_version(' there') } }
  puts "fast_version"
  puts allocate_count { 1000.times { fast_version } }
  puts "fast_version with arg"
  puts allocate_count { 1000.times { fast_version(' there') } }

  Benchmark.ips do |x|
    x.report("master_version") { master_version }
    x.report("master_version with arg") { master_version(' there') }
    x.report("fast_version")     { fast_version }
    x.report("fast_version with arg")     { fast_version(' there') }
    x.compare!
  end
end

test

results:

master_version
{:FREE=>-1981, :T_STRING=>2052}
master_version with arg
{:FREE=>-1001, :T_STRING=>1000}
fast_version
{:FREE=>-1001, :T_STRING=>1000}
fast_version with arg
{:FREE=>-1001, :T_STRING=>1000}
Warming up --------------------------------------
      master_version   138.851k i/100ms
master_version with arg
                       164.029k i/100ms
        fast_version   165.737k i/100ms
fast_version with arg
                       167.016k i/100ms
Calculating -------------------------------------
      master_version      2.464M (±14.7%) i/s -     11.941M in   5.023307s
master_version with arg
                          3.754M (± 8.5%) i/s -     18.699M in   5.021354s
        fast_version      3.449M (±11.7%) i/s -     17.071M in   5.033312s
fast_version with arg
                          3.636M (± 6.9%) i/s -     18.205M in   5.034792s

Comparison:
master_version with arg:  3753896.1 i/s
fast_version with arg:  3636094.5 i/s - same-ish: difference falls within error
        fast_version:  3448766.2 i/s - same-ish: difference falls within error
      master_version:  2463857.3 i/s - 1.52x  slower

@pixeltrix
Copy link
Contributor

I think it'd be fine to use an inline ternary here to save adding an extra method, e.g:

"#{sanitized_method_name}#{multiple ? '[]' : ''}"

it's also shorter than the method call.

"my string #{nil}" results in an additional '' string allocation, I'm
guessing because the nil has to be converted to a string.

"my string #{'[]' if multiple}" results in "my string #{nil}" if
multiple is false. Doing "my string #{''}" does not result in an extra
string allocation. I moved the if multiple logic into a method so I only
had to make the change once.

```ruby
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 "benchmark-ips"
  gem "rails"
end

def allocate_count
  GC.disable
  before = ObjectSpace.count_objects
  yield
  after = ObjectSpace.count_objects
  after.each { |k,v| after[k] = v - before[k] }
  after[:T_HASH] -= 1 # probe effect - we created the before hash.
  GC.enable
  result = after.reject { |k,v| v == 0 }
  GC.start
  result
end

@html_options = {}

def master_version(multiple=nil)
  "hi#{"[]" if multiple}"
end

def fast_version(multiple=nil)
  str = multiple ? "[]" : ''
  "hi#{str}"
end

def test
  puts "master_version"
  puts allocate_count { 1000.times { master_version } }
  puts "master_version with arg"
  puts allocate_count { 1000.times { master_version(' there') } }
  puts "fast_version"
  puts allocate_count { 1000.times { fast_version } }
  puts "fast_version with arg"
  puts allocate_count { 1000.times { fast_version(' there') } }

  Benchmark.ips do |x|
    x.report("master_version") { master_version }
    x.report("master_version with arg") { master_version(' there') }
    x.report("fast_version")     { fast_version }
    x.report("fast_version with arg")     { fast_version(' there') }
    x.compare!
  end
end

test
```

results:
```ruby
master_version
{:FREE=>-1981, :T_STRING=>2052}
master_version with arg
{:FREE=>-1001, :T_STRING=>1000}
fast_version
{:FREE=>-1001, :T_STRING=>1000}
fast_version with arg
{:FREE=>-1001, :T_STRING=>1000}
Warming up --------------------------------------
      master_version   138.851k i/100ms
master_version with arg
                       164.029k i/100ms
        fast_version   165.737k i/100ms
fast_version with arg
                       167.016k i/100ms
Calculating -------------------------------------
      master_version      2.464M (±14.7%) i/s -     11.941M in   5.023307s
master_version with arg
                          3.754M (± 8.5%) i/s -     18.699M in   5.021354s
        fast_version      3.449M (±11.7%) i/s -     17.071M in   5.033312s
fast_version with arg
                          3.636M (± 6.9%) i/s -     18.205M in   5.034792s

Comparison:
master_version with arg:  3753896.1 i/s
fast_version with arg:  3636094.5 i/s - same-ish: difference falls within error
        fast_version:  3448766.2 i/s - same-ish: difference falls within error
      master_version:  2463857.3 i/s - 1.52x  slower
```
@dillonwelch
Copy link
Contributor Author

Fixed

@pixeltrix pixeltrix merged commit c7cbc2e into rails:master Mar 20, 2018
@dillonwelch dillonwelch deleted the perf-improvement-tag-name branch March 20, 2018 14:18
dillonwelch added a commit to q-centrix/rails that referenced this pull request Mar 20, 2018
"my string #{nil}" results in an additional '' string allocation, I'm
guessing because the nil has to be converted to a string.

"my string #{'[]' if multiple}" results in "my string #{nil}" if
multiple is false. Doing "my string #{''}" does not result in an extra
string allocation.

This code mainly seems to be executed during the first page load of an
app. For my app, this saved 3698 allocations of "" - around 150 KB of
savings.

See rails#32302

Benchmarking results:
```

results:
```ruby
master_version
{:FREE=>-1981, :T_STRING=>2052}
master_version with arg
{:FREE=>-1001, :T_STRING=>1000}
fast_version
{:FREE=>-1001, :T_STRING=>1000}
fast_version with arg
{:FREE=>-1001, :T_STRING=>1000}
Warming up --------------------------------------
      master_version   138.851k i/100ms
master_version with arg
                       164.029k i/100ms
        fast_version   165.737k i/100ms
fast_version with arg
                       167.016k i/100ms
Calculating -------------------------------------
      master_version      2.464M (±14.7%) i/s -     11.941M in   5.023307s
master_version with arg
                          3.754M (± 8.5%) i/s -     18.699M in   5.021354s
        fast_version      3.449M (±11.7%) i/s -     17.071M in   5.033312s
fast_version with arg
                          3.636M (± 6.9%) i/s -     18.205M in   5.034792s

Comparison:
master_version with arg:  3753896.1 i/s
fast_version with arg:  3636094.5 i/s - same-ish: difference falls within error
        fast_version:  3448766.2 i/s - same-ish: difference falls within error
      master_version:  2463857.3 i/s - 1.52x  slower
```

Benchmarking code:
```ruby
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 "benchmark-ips"
  gem "rails"
end

def allocate_count
  GC.disable
  before = ObjectSpace.count_objects
  yield
  after = ObjectSpace.count_objects
  after.each { |k,v| after[k] = v - before[k] }
  after[:T_HASH] -= 1 # probe effect - we created the before hash.
  GC.enable
  result = after.reject { |k,v| v == 0 }
  GC.start
  result
end

@html_options = {}

def master_version(multiple=nil)
  "hi#{"[]" if multiple}"
end

def fast_version(multiple=nil)
  str = multiple ? "[]" : ''
  "hi#{str}"
end

def test
  puts "master_version"
  puts allocate_count { 1000.times { master_version } }
  puts "master_version with arg"
  puts allocate_count { 1000.times { master_version(' there') } }
  puts "fast_version"
  puts allocate_count { 1000.times { fast_version } }
  puts "fast_version with arg"
  puts allocate_count { 1000.times { fast_version(' there') } }

  Benchmark.ips do |x|
    x.report("master_version") { master_version }
    x.report("master_version with arg") { master_version(' there') }
    x.report("fast_version")     { fast_version }
    x.report("fast_version with arg")     { fast_version(' there') }
    x.compare!
  end
end

test
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants