Freeze strings to prevent re-allocation of newline/empty strings #961

dillonwelch · 2017-10-20T19:31:05Z

Most of these were discovered when profiling performance issues on a page with 1000s of links generated via link_to in Rails.

dillonwelch · 2017-10-20T19:31:51Z

lib/haml/compiler.rb

+        # bin/rails test test/helper_test.rb:596
+        # bin/rails test test/engine_test.rb:534
+        #
+        # Both appear to be testing outer whitespace nuking but at this point


If anyone has insight into this issue and is willing to help me fix the code/test to be able to freeze this string that would be much appreciated! 😍

Unfortunately I have no time to assist you, but I would write many binding.prys to see how string that shouldn't be freezed is freezed.

dillonwelch · 2017-10-20T19:32:46Z

lib/haml/helpers/action_view_mods.rb

@@ -55,7 +55,9 @@ module TagHelper
      def content_tag_with_haml(name, *args, &block)
        return content_tag_without_haml(name, *args, &block) unless is_haml?

-        preserve = haml_buffer.options.fetch(:preserve, %w[textarea pre code]).include?(name.to_s)
+        @_content_tag_name_cache ||= {}
+        name_string = @_content_tag_name_cache[name] ||= name.to_s


Idea here being that we only needed to run to_s on each content tag once. In particular, I was getting a in here a lot from links. Any better ideas are welcome.

Performance is within margin of error but saves a string allocation for each time this method is called.

Warming up -------------------------------------- master_version 188.121k i/100ms fast_version 201.722k i/100ms Calculating ------------------------------------- master_version 5.176M (±10.1%) i/s - 25.584M in 5.007528s fast_version 5.595M (± 7.3%) i/s - 27.838M in 5.004395s Comparison: fast_version: 5595000.8 i/s master_version: 5176423.8 i/s - same-ish: difference falls within error

Having %w[textarea pre code] as constant would be enough. I guess main performance gain comes from that. I'm reluctant to introduce ivar in template evaluation environment, and in general cache is likely to introduce bug.

@k0kubun I went ahead and added the constant for %w[textarea pre code] in #966. Additionally, what do you think about adding a constant for common content_tag options to cache the name.to_s call? Mainly the things that show up in this search of the Rails repo and any other tag suggestions you have?

Your above benchmark is now expired after you add constant, so you should put benchmark again.

As I said,

I'm reluctant to introduce ivar in template evaluation environment, and in general cache is likely to introduce bug.

So at least I wouldn't merge it unless there's SIGNIFICANT performance benefit in benchmark. Basically to_s is not slow as the constant includes only string.

dillonwelch · 2017-10-20T19:36:56Z

Just noticed the Haml has two primary branches. stable is where the development of the released version (currently 5.0.4) takes place. This is where most bug fixes should go. Master, on the other hand, is where the next version of Haml is being developed. This is the place for new features. in the guidelines. Not sure if this is considered a bugfix or not? If so, I can re-make the PR against stable.

In micro-benchmarking, this change performs within margin of error of the existing code but saves a string allocation each time the method is called (assuming the quote strings in the old code were frozen). ``` 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" 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 interpolated = "hi" puts "'"' + interpolated + '"'" puts allocate_count { 1000.times { '"' + interpolated + '"' } } puts "%(\"\#{interpolated}\")" puts allocate_count { 1000.times { %("#{interpolated}") } } puts "\"\#{interpolated}\"" puts allocate_count { 1000.times { "\"#{interpolated}\"" } } Benchmark.ips do |x| x.report("'"' + interpolated + '"'") { '"' + interpolated + '"' } x.report("%(\"\#{interpolated}\")") { %("#{interpolated}") } x.report("\"\#{interpolated}\"") { "\"#{interpolated}\"" } x.compare! end ```ruby ``` ' + interpolated + ' {:FREE=>-1892, :T_STRING=>2052} %("#{interpolated}") {:FREE=>-1001, :T_STRING=>1000} "#{interpolated}" {:FREE=>-1001, :T_STRING=>1000} Warming up -------------------------------------- ' + interpolated + ' 81.706k i/100ms %("#{interpolated}") 106.128k i/100ms "#{interpolated}" 137.855k i/100ms Calculating ------------------------------------- ' + interpolated + ' 3.892M (±23.2%) i/s - 17.975M in 5.007068s %("#{interpolated}") 3.722M (±17.3%) i/s - 17.830M in 5.022549s "#{interpolated}" 3.725M (±15.0%) i/s - 18.059M in 5.023493s Comparison: ' + interpolated + ': 3892392.6 i/s "#{interpolated}": 3725385.8 i/s - same-ish: difference falls within error %("#{interpolated}"): 3722401.7 i/s - same-ish: difference falls within error ```

dillonwelch · 2017-10-23T23:55:37Z

lib/haml/util.rb

            balance(scan, ?{, ?}, 1)[0][0...-1]
          else
            scan.scan(/\w+/)
          end
-          content = eval('"' + interpolated + '"')
-          content.prepend(char) if char == '@' || char == '$'
+          content = eval("\"#{interpolated}\"")


See 42f4eaa for justification, summary is that it saves a string allocation each time this code is called for roughly the same performance.

@old

Stringifying the keys of a hash can be done without allocating many arrays like the previous approach did. ```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" 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 @old = {a: :b, c: :d, e: :f} def master_version Hash[@old.map { |k, v| [k.to_s, v] }] end def fast_version result = {} @old.each { |k, v| result[k.to_s] = v } 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 ``` ```ruby master_version {:FREE=>-14768, :T_STRING=>6054, :T_ARRAY=>7000, :T_HASH=>1000, :T_IMEMO=>1000} fast_version {:FREE=>-7001, :T_STRING=>6000, :T_HASH=>1000} Warming up -------------------------------------- master_version 38.137k i/100ms fast_version 50.133k i/100ms Calculating ------------------------------------- master_version 451.898k (±19.2%) i/s - 2.174M in 5.002186s fast_version 633.579k (±19.4%) i/s - 3.058M in 5.019391s Comparison: fast_version: 633578.7 i/s master_version: 451897.6 i/s - same-ish: difference falls within error ```

k0kubun

Please use # frozen_string_literal: true instead of "str".freeze.

k0kubun · 2017-10-24T15:13:16Z

Any performance improvement uglifying code should include benchmark to justify its introduction. But you only attached #961 (comment).

Basically this change is for compilation, so I want compilation benchmark. Time of compilation + rendering or some rendering benchmark that uses dynamic helper is also welcomed.

dillonwelch · 2017-10-25T00:39:48Z

I will go ahead and separate the other performance changes unrelated to frozen strings into separate PRs so we can discuss separately and I can more easily post benchmarks.

Unfortunately all of these files have strings that are mutated in place making the frozen_string_literal: true magic comment break tests. See #962. Here's an example from parser.rb. I'm down to assist in rewriting these over time but I don't have the time to do it all at once and I would need some thought partnership in understanding the context of the code in each file.

What do you think about merging the "str".freeze changes and then reverting them as we're able to rewrite the files to allow for all strings to be frozen?

k0kubun · 2017-10-25T07:42:20Z

What do you think about merging the "str".freeze changes and then reverting them as we're able to rewrite the files to allow for all strings to be frozen?

There's no reason to introduce intermediate change for development in master. That's because I've never seen that performance is optimized enough to introduce ugly syntax "str".freeze, except in very micro benchmark like rendering template's compiled code. I'm only okay for # frozen_string_literal: true pragma as it's not so ugly.

Just try adding .dup to all string literals and gradually remove them, and make PR after it's done.
(Ideally I want to recommend String#+@ for such purpose but Haml is supporting Ruby 2.0. Personally I'm against introducing many .dups too, but at least it's shorter.)

And in any situation, any performance improvement without benchmark isn't acceptable.

k0kubun · 2017-10-26T05:56:29Z

please rebase against master

k0kubun · 2017-10-26T06:26:39Z

Ah, this PR is split to multiple PRs. Nice! Closing this.

Freeze strings to prevent re-allocation of newline/empty strings

f70a7e5

dillonwelch commented Oct 20, 2017

View reviewed changes

CHANGELOG.md note

6858136

dillonwelch added 2 commits October 20, 2017 13:52

Freeze even more strings

b939015

dillonwelch commented Oct 23, 2017

View reviewed changes

Roll back changes to use array joins because it's slower

de05e61

dillonwelch mentioned this pull request Oct 24, 2017

frozen_string_literal: false #962

Closed

k0kubun requested changes Oct 24, 2017

View reviewed changes

k0kubun closed this Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Freeze strings to prevent re-allocation of newline/empty strings #961

Freeze strings to prevent re-allocation of newline/empty strings #961

dillonwelch commented Oct 20, 2017

dillonwelch Oct 20, 2017

k0kubun Oct 24, 2017

dillonwelch Oct 20, 2017

dillonwelch Oct 24, 2017

k0kubun Oct 24, 2017

dillonwelch Oct 26, 2017 •

edited

k0kubun Oct 26, 2017 •

edited

dillonwelch commented Oct 20, 2017

dillonwelch Oct 23, 2017

k0kubun left a comment

k0kubun commented Oct 24, 2017

dillonwelch commented Oct 25, 2017

k0kubun commented Oct 25, 2017 •

edited

k0kubun commented Oct 26, 2017

k0kubun commented Oct 26, 2017

Freeze strings to prevent re-allocation of newline/empty strings #961

Freeze strings to prevent re-allocation of newline/empty strings #961

Conversation

dillonwelch commented Oct 20, 2017

dillonwelch Oct 20, 2017

Choose a reason for hiding this comment

k0kubun Oct 24, 2017

Choose a reason for hiding this comment

dillonwelch Oct 20, 2017

Choose a reason for hiding this comment

dillonwelch Oct 24, 2017

Choose a reason for hiding this comment

k0kubun Oct 24, 2017

Choose a reason for hiding this comment

dillonwelch Oct 26, 2017 • edited

Choose a reason for hiding this comment

k0kubun Oct 26, 2017 • edited

Choose a reason for hiding this comment

dillonwelch commented Oct 20, 2017

dillonwelch Oct 23, 2017

Choose a reason for hiding this comment

k0kubun left a comment

Choose a reason for hiding this comment

k0kubun commented Oct 24, 2017

dillonwelch commented Oct 25, 2017

k0kubun commented Oct 25, 2017 • edited

k0kubun commented Oct 26, 2017

k0kubun commented Oct 26, 2017

dillonwelch Oct 26, 2017 •

edited

k0kubun Oct 26, 2017 •

edited

k0kubun commented Oct 25, 2017 •

edited