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

Improve String#strip_heredoc #21596

Merged
merged 1 commit into from Sep 12, 2015
Merged

Improve String#strip_heredoc #21596

merged 1 commit into from Sep 12, 2015

Conversation

@JuanitoFatas
Copy link
Contributor

JuanitoFatas commented Sep 12, 2015

Summary

Saves about 6 MB, about 40% faster.

Benchmark Scripts

strip_heredoc.rb

require "active_support/core_ext/object/try"
require "get_process_mem"

class String
  def strip_heredoc
    indent = scan(/^[ \t]*(?=\S)/).min.try(:size) || 0
    gsub(/^[ \t]{#{indent}}/, '')
  end
end

if ENV["MEASURE_MEMORY"] == "yes"
  mem = GetProcessMem.new
  GC.start
  GC.disable
  10000.times do
    <<-MSG.strip_heredoc
      xhr and xml_http_request methods are deprecated in favor of
      `get :index, xhr: true` and `post :create, xhr: true`
    MSG
  end
  before = mem.mb

  after = mem.mb
  GC.enable
  puts "Before: #{before} MiB"
  puts "After: #{after} MiB"
  puts "Diff: #{after - before} MiB"
end

patched_strip_heredoc.rb

require "active_support/core_ext/object/try"
require "get_process_mem"

class String
  def patched_strip_heredoc
    gsub(/^#{scan(/^[ \t]*(?=\S)/).min}/, "".freeze)
  end
end

if ENV["MEASURE_MEMORY"] == "yes"
  mem = GetProcessMem.new
  GC.start
  GC.disable
  10000.times do
    <<-MSG.patched_strip_heredoc
      xhr and xml_http_request methods are deprecated in favor of
      `get :index, xhr: true` and `post :create, xhr: true`
    MSG
  end
  before = mem.mb

  after = mem.mb
  GC.enable
  puts "Before: #{before} MiB"
  puts "After: #{after} MiB"
  puts "Diff: #{after - before} MiB"
end

Memory Improvement

Before

$ MEASURE_MEMORY=yes ruby strip_heredoc.rb
Before: 44.73828125 MiB
After: 44.7734375 MiB
Diff: 0.03515625 MiB

After

$ MEASURE_MEMORY=yes ruby patched_strip_heredoc.rb
Before: 37.9765625 MiB
After: 38.015625 MiB
Diff: 0.0390625 MiB

44.7734375 - 38.015625 = 6.75

=> Saves about 6.75 MiB

Performance Improvement

benchmark.rb

require "benchmark/ips"
require_relative "./strip_heredoc"
require_relative "./patched_strip_heredoc"

def original
  <<-MSG.strip_heredoc
    xhr and xml_http_request methods are deprecated in favor of
    `get :index, xhr: true` and `post :create, xhr: true`
  MSG
end

def patched
  <<-MSG.patched_strip_heredoc
    xhr and xml_http_request methods are deprecated in favor of
    `get :index, xhr: true` and `post :create, xhr: true`
  MSG
end

Benchmark.ips do |x|
  x.report("original") { original }
  x.report(" patched") { patched  }
  x.compare!
end
$ ruby -v benchmark.rb
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-darwin14]
Calculating -------------------------------------
            original     5.652k i/100ms
             patched     6.477k i/100ms
-------------------------------------------------
            original     54.076k (± 5.7%) i/s -    271.296k
             patched     74.557k (± 6.2%) i/s -    375.666k

Comparison:
             patched:    74557.0 i/s
            original:    54076.4 i/s - 1.38x slower

=> About 38% faster

Compatibility

  1. Clone rails project git clone git@github.com:rails/rails.git
  2. Apply this patch to
    activesupport/lib/active_support/core_ext/string/strip.rb
  3. cd activesupport
  4. run rake
  5. And tests passed:
➜ activesupport $ rake
/Users/Juan/.rubies/ruby-2.2.2/bin/ruby -w -I"lib:test"
"/Users/Juan/.rubies/ruby-2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb"
 "test/**/*_test.rb"

# Running:

........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
......................................................................S.
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS

Finished in 15.343004s, 214.2344 runs/s, 24902.4898 assertions/s.

3287 runs, 382079 assertions, 0 failures, 0 errors, 48 skips

You have skipped tests. Run with --verbose for details.
@fxn

This comment has been minimized.

Copy link
Member

fxn commented Sep 12, 2015

Excellent.

This patch is not equivalent, it is stronger.

The original implementation computes the length of the indent in the least indented line, and removes those much whitespace characters (space or tab in this case) from the rest of lines.

This alternative looks at the shorter indent and replaces it as a string. Thus, assumes all lines are indented in the same way.

The approaches defer when the indentation mixes spaces and tabs, this alternative assumes the indentation is uniform. But I think this assumption is correct, this helper is thought for the use case in which the indentation is uniform, the behavior when it is mixed... doesn't matter. I mean, any choice is valid.

I am 👍 on this patch, but would like to ask for an stylistic tweak. In Rails we use single quotes unless double quotes are needed, would you mind changing the quotes?

Saves about 6 MB, about 40% faster.

**strip_heredoc.rb**

```ruby
require "active_support/core_ext/object/try"
require "get_process_mem"

class String
  def strip_heredoc
    indent = scan(/^[ \t]*(?=\S)/).min.try(:size) || 0
    gsub(/^[ \t]{#{indent}}/, '')
  end
end

if ENV["MEASURE_MEMORY"] == "yes"
  mem = GetProcessMem.new
  GC.start
  GC.disable
  10000.times do
    <<-MSG.strip_heredoc
      xhr and xml_http_request methods are deprecated in favor of
      `get :index, xhr: true` and `post :create, xhr: true`
    MSG
  end
  before = mem.mb

  after = mem.mb
  GC.enable
  puts "Before: #{before} MiB"
  puts "After: #{after} MiB"
  puts "Diff: #{after - before} MiB"
end
```

**patched_strip_heredoc.rb**

```ruby
require "active_support/core_ext/object/try"
require "get_process_mem"

class String
  def patched_strip_heredoc
    gsub(/^#{scan(/^[ \t]*(?=\S)/).min}/, "".freeze)
  end
end

if ENV["MEASURE_MEMORY"] == "yes"
  mem = GetProcessMem.new
  GC.start
  GC.disable
  10000.times do
    <<-MSG.patched_strip_heredoc
      xhr and xml_http_request methods are deprecated in favor of
      `get :index, xhr: true` and `post :create, xhr: true`
    MSG
  end
  before = mem.mb

  after = mem.mb
  GC.enable
  puts "Before: #{before} MiB"
  puts "After: #{after} MiB"
  puts "Diff: #{after - before} MiB"
end
```

**Before**

```
$ MEASURE_MEMORY=yes ruby strip_heredoc.rb
Before: 44.73828125 MiB
After: 44.7734375 MiB
Diff: 0.03515625 MiB
```

**After**

```
$ MEASURE_MEMORY=yes ruby patched_strip_heredoc.rb
Before: 37.9765625 MiB
After: 38.015625 MiB
Diff: 0.0390625 MiB
```

`44.7734375 -  38.015625 = 6.75`

=> **Saves about 6.75 MiB**

**benchmark.rb**

```ruby
require "benchmark/ips"
require_relative "./strip_heredoc"
require_relative "./patched_strip_heredoc"

def original
  <<-MSG.strip_heredoc
    xhr and xml_http_request methods are deprecated in favor of
    `get :index, xhr: true` and `post :create, xhr: true`
  MSG
end

def patched
  <<-MSG.patched_strip_heredoc
    xhr and xml_http_request methods are deprecated in favor of
    `get :index, xhr: true` and `post :create, xhr: true`
  MSG
end

Benchmark.ips do |x|
  x.report("original") { original }
  x.report(" patched") { patched  }
  x.compare!
end
```

```
$ ruby -v benchmark.rb
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-darwin14]
Calculating -------------------------------------
            original     5.652k i/100ms
             patched     6.477k i/100ms
-------------------------------------------------
            original     54.076k (± 5.7%) i/s -    271.296k
             patched     74.557k (± 6.2%) i/s -    375.666k

Comparison:
             patched:    74557.0 i/s
            original:    54076.4 i/s - 1.38x slower
```

=> **About 38% faster**

1. Clone rails project `git clone git@github.com:rails/rails.git`
2. Apply this patch to
`activesupport/lib/active_support/core_ext/string/strip.rb`
3. `cd activesupport`
4. run `rake`
5. And tests passed:

```
➜ activesupport $ rake
/Users/Juan/.rubies/ruby-2.2.2/bin/ruby -w -I"lib:test"
"/Users/Juan/.rubies/ruby-2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb"
 "test/**/*_test.rb"

........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
........................................................................
......................................................................S.
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS

Finished in 15.343004s, 214.2344 runs/s, 24902.4898 assertions/s.

3287 runs, 382079 assertions, 0 failures, 0 errors, 48 skips

You have skipped tests. Run with --verbose for details.
```
@JuanitoFatas

This comment has been minimized.

Copy link
Contributor Author

JuanitoFatas commented Sep 12, 2015

@fxn Thanks for the detailed sum up! 😊

I am on this patch, but would like to ask for an stylistic tweak. In Rails we use single quotes unless double quotes are needed, would you mind changing the quotes?

Changed to single quote. 🐬

@fxn

This comment has been minimized.

Copy link
Member

fxn commented Sep 12, 2015

🚀 ❤️

fxn added a commit that referenced this pull request Sep 12, 2015
Improve String#strip_heredoc
@fxn fxn merged commit 59ec70d into rails:master Sep 12, 2015
@JuanitoFatas JuanitoFatas deleted the JuanitoFatas:perf/strip-heredoc branch Sep 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.