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

Frozen string literal changes #967

Merged
merged 11 commits into from Oct 29, 2017

Conversation

Projects
None yet
3 participants
@oniofchaos
Copy link
Contributor

commented Oct 26, 2017

I was able to get frozen_string_literal: true turned on in two files. Unfortunately I wasn't able to get them on in the rest, the majority of which were because of various calls to rstrip! and tr! which are more performant in micro-benchmarks and in memory usage. I made other changes in these files to string builder logic such that they no longer mutate strings, based on this StackOverflow post.

String mutations preventing frozen_string_literal: true
lib/haml/compiler.rb - rstrip!
lib/haml/filters - rstrip!
lib/haml/template_engine.rb - force_encoding, tr!
lib/haml/parser.rb - rstrip!

I wanted to keep the commit history for discussion about the various changes. Once everything is approved I can squash the commits and add a changelog entry.

Result of ruby benchmark.rb

                                         Haml |     ERB |   Erubi |
-------------------------------------------------------------------
Cached                                  0.088 |   0.073 |   0.051 |
ActionView                             15.558 |         |  10.493 |
ActionView with deep partials          41.512 |         |  37.338 |

Current master

                                         Haml |     ERB |   Erubi |
-------------------------------------------------------------------
Cached                                  0.085 |   0.073 |   0.070 |
ActionView                             18.001 |         |  10.954 |
ActionView with deep partials          46.022 |         |  34.406 |
@temple_engine.precompiled_with_ambles(local_names) << "}\n", scope, @options.filename, @options.line)
str = @temple_engine.precompiled_with_ambles(local_names)
eval(
"Proc.new { |*_haml_locals| _haml_locals = _haml_locals[0] || {}; #{str}}\n",

This comment has been minimized.

Copy link
@oniofchaos

oniofchaos Oct 26, 2017

Author Contributor

This change is faster on micro-benchmarking as well as saving memory (2 strings per method call).

# frozen_string_literal: false
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

def master_version
  "hi there" << "cool" << "yeah"
end

def fast_version
  "#{'hi there'}#{'cool'}#{'yeah'}"
end

puts "master_version"
puts allocate_count { 1000.times { master_version } }
puts "fast_version"
puts allocate_count { 1000.times { fast_version } }

Benchmark.ips do |x|
  x.report("master_version") { master_version }
  x.report("fast_version")     { fast_version }
  x.compare!
end
master_version
{:FREE=>-2846, :T_STRING=>3052}
fast_version
{:FREE=>-1001, :T_STRING=>1000}
Warming up --------------------------------------
      master_version   120.220k i/100ms
        fast_version   181.848k i/100ms
Calculating -------------------------------------
      master_version      2.527M (±14.8%) i/s -     12.383M in   5.039857s
        fast_version      6.634M (±17.6%) i/s -     32.005M in   5.002381s

Comparison:
        fast_version:  6633843.3 i/s
      master_version:  2526801.8 i/s - 2.63x  slower
s << '|' unless s.empty?
s << Regexp.escape(t)
end
tags = tags.map { |tag| Regexp.escape(tag) }.join('|')

This comment has been minimized.

Copy link
@oniofchaos

oniofchaos Oct 26, 2017

Author Contributor

Doesn't impact performance or memory either way but was necessary for the frozen_string_literal: true change

# frozen_string_literal: false
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

@tags = %w(hi there I am a list of tags)

def master_version
  @tags.each_with_object('') do |t, s|
    s << '|'.freeze unless s.empty?
    s << Regexp.escape(t)
  end
end

def fast_version
  @tags.map { |tag| Regexp.escape(tag) }.join('|'.freeze)
end

puts "master_version"
puts allocate_count { 1000.times { master_version } }
puts "fast_version"
puts allocate_count { 1000.times { fast_version } }

Benchmark.ips do |x|
  x.report("master_version") { master_version }
  x.report("fast_version")     { fast_version }
  x.compare!
end
master_version
{:FREE=>-9790, :T_STRING=>9052, :T_IMEMO=>1001}
fast_version
{:FREE=>-10001, :T_STRING=>9000, :T_ARRAY=>1000}
Warming up --------------------------------------
      master_version    13.960k i/100ms
        fast_version    16.597k i/100ms
Calculating -------------------------------------
      master_version    200.727k (±15.4%) i/s -    991.160k in   5.057382s
        fast_version    206.033k (±11.7%) i/s -      1.029M in   5.072112s

Comparison:
        fast_version:   206033.4 i/s
      master_version:   200727.0 i/s - same-ish: difference falls within error

@@ -698,7 +698,7 @@ def parse_new_attributes(text)
end

static_attributes = {}
dynamic_attributes = "{"
dynamic_attributes = ["{"]

This comment has been minimized.

Copy link
@oniofchaos

oniofchaos Oct 26, 2017

Author Contributor

Doesn't impact performance or memory either way but is necessary for future future_string_literal: true changes

# frozen_string_literal: false
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

def master_version
  dynamic_attributes = '{'
  if rand(2) == 1
    dynamic_attributes << 'hi there'.freeze
  end
  dynamic_attributes << "}".freeze
  dynamic_attributes = nil if dynamic_attributes == "{}".freeze
end

def fast_version
  dynamic_attributes = ['{'.freeze]
  if rand(2) == 1
    dynamic_attributes << 'hi there'.freeze
  end
  dynamic_attributes << "}".freeze
  dynamic_attributes = nil if dynamic_attributes.first == "{}".freeze && dynamic_attributes.length == 1
end

puts "master_version"
puts allocate_count { 1000.times { master_version } }
puts "fast_version"
puts allocate_count { 1000.times { fast_version } }

Benchmark.ips do |x|
  x.report("master_version") { master_version }
  x.report("fast_version")     { fast_version }
  x.compare!
end
master_version
{:FREE=>-693, :T_STRING=>1052, :T_IMEMO=>1}
fast_version
{:FREE=>-1001, :T_ARRAY=>1000}
Warming up --------------------------------------
      master_version   111.400k i/100ms
        fast_version   122.259k i/100ms
Calculating -------------------------------------
      master_version      1.976M (±17.1%) i/s -      9.580M in   5.056250s
        fast_version      1.966M (±25.6%) i/s -      8.925M in   5.017136s

Comparison:
      master_version:  1975829.5 i/s
        fast_version:  1965749.8 i/s - same-ish: difference falls within error

This comment has been minimized.

Copy link
@k0kubun

k0kubun Oct 26, 2017

Member

Thanks for your effort of gradual changes and to put benchmark, but please exclude intermediate changes that have no benefit. It makes hard to read commit log to know why this change was necessary, and this change should be committed with # frozen_string_literal: true.

This comment has been minimized.

Copy link
@k0kubun

k0kubun Oct 26, 2017

Member

And there's no need to change this to Array buffer for introducing # frozen_string_literal: true.

It uglifies code after this, so please use "{".dup instead.

@@ -200,8 +197,8 @@ def preserve(input = nil, &block)
# @yield [item] A block which contains Haml code that goes within list items
# @yieldparam item An element of `enum`
def list_of(enum, opts={}, &block)
opts_attributes = opts.each_with_object('') {|(k, v), s| s << " #{k}='#{v}'"}
enum.each_with_object('') do |i, ret|
opts_attributes = opts.map { |k, v| " #{k}='#{v}'" }.join

This comment has been minimized.

Copy link
@oniofchaos

oniofchaos Oct 26, 2017

Author Contributor

Same as above

# frozen_string_literal: false
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

@strings = %w(hi there I am an array of strings)

def master_version
  @strings.each_with_object('') do |string, builder|
    builder << "#{string} is cool "
  end
end

def fast_version
  @strings.map do |string|
    "#{string} is cool "
  end.join
end

puts "master_version"
puts allocate_count { 1000.times { master_version } }
puts "fast_version"
puts allocate_count { 1000.times { fast_version } }

Benchmark.ips do |x|
  x.report("master_version") { master_version }
  x.report("fast_version")     { fast_version }
  x.compare!
end
master_version
{:FREE=>-9836, :T_STRING=>9052, :T_IMEMO=>1001}
fast_version
{:FREE=>-10001, :T_STRING=>9000, :T_ARRAY=>1000}
Warming up --------------------------------------
      master_version    23.988k i/100ms
        fast_version    29.611k i/100ms
Calculating -------------------------------------
      master_version    302.295k (± 8.2%) i/s -      1.511M in   5.034657s
        fast_version    359.883k (±12.2%) i/s -      1.777M in   5.015867s

Comparison:
        fast_version:   359883.3 i/s
      master_version:   302295.1 i/s - same-ish: difference falls within error
ret << %Q!<li#{opts_attributes}>#{result}</li>!
end
%Q!<li#{opts_attributes}>#{result}</li>!
end.join("\n")

This comment has been minimized.

Copy link
@oniofchaos

oniofchaos Oct 26, 2017

Author Contributor

Same as above

# frozen_string_literal: false
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

@strings = %w(hi there I am an array of strings)

def master_version
  @strings.each_with_object('') do |string, builder|
    builder << "\n".freeze unless builder.empty?
    builder << "#{string} is cool "
  end
end

def fast_version
  @strings.map do |string|
    "#{string} is cool "
  end.join("\n".freeze)
end

puts "master_version"
puts allocate_count { 1000.times { master_version } }
puts "fast_version"
puts allocate_count { 1000.times { fast_version } }

Benchmark.ips do |x|
  x.report("master_version") { master_version }
  x.report("fast_version")     { fast_version }
  x.compare!
end
master_version
{:FREE=>-9773, :T_STRING=>9052, :T_IMEMO=>1001}
fast_version
{:FREE=>-10001, :T_STRING=>9000, :T_ARRAY=>1000}
Warming up --------------------------------------
      master_version    23.523k i/100ms
        fast_version    18.124k i/100ms
Calculating -------------------------------------
      master_version    271.699k (± 9.1%) i/s -      1.364M in   5.074300s
        fast_version    280.265k (±13.2%) i/s -      1.377M in   5.012751s

Comparison:
        fast_version:   280265.0 i/s
      master_version:   271698.9 i/s - same-ish: difference falls within error
@@ -737,8 +741,10 @@ def parse_new_attribute(scanner)
end

return name, [:static, content.first[1]] if content.size == 1
return name, [:dynamic,
%!"#{content.each_with_object('') {|(t, v), s| s << (t == :str ? inspect_obj(v)[1...-1] : "\#{#{v}}")}}"!]

This comment has been minimized.

Copy link
@k0kubun

k0kubun Oct 26, 2017

Member

Please don't introduce unnecessary changes. Obviously we can enable # frozen_string_literal: true if we change '' to ''.dup.

@oniofchaos

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2017

New benchmark:

                                         Haml |     ERB |   Erubi |
-------------------------------------------------------------------
Cached                                  0.073 |   0.117 |   0.076 |
ActionView                             17.292 |         |  10.315 |
ActionView with deep partials          38.455 |         |  38.641 |
@k0kubun

This comment has been minimized.

Copy link
Member

commented Oct 29, 2017

With ruby 2.5.0dev (2017-10-27 trunk 60479) [x86_64-darwin16],

before

                                         Haml |     
-----------------------------------------------
Cached                                  0.059 |         
ActionView                             11.794 |         
ActionView with deep partials          30.832 |         

after

                                         Haml |    
-----------------------------------------------
Cached                                  0.061 |         
ActionView                             11.706 |         
ActionView with deep partials          31.120 |         

Probably this PR doesn't approach this benchmark's hotspot.

But in general I like to have # frozen_string_literal: true to avoid future performance regression, and I feel changed code is more readable (except ''.dup part though). So I'm merging this. Thanks for your work!

@k0kubun k0kubun merged commit 1d446b3 into haml:master Oct 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@oniofchaos oniofchaos deleted the q-centrix:frozen-string-literal branch Oct 30, 2017

@oniofchaos

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

Excellent. Glad to help.

What's the general release/version bump schedule?

@k0kubun

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

@amatsuda I'm in favor of releasing current one as new version.

Changes are not breaking but making things to be frozen might be dangerous. I think it's time to cut 5.1.0 release.

@amatsuda

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

Hm... I can do 5.0.5 release. I don't think this is worth a minor version bump since we don't really have any new user-facing feature nor breaking change.

@k0kubun

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

I see, it was not so strong opinion. 5.0.5 is okay for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.