From f5f0b49b9b233f85664d7a464c18afb2f3f10692 Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Thu, 26 Oct 2017 18:17:38 -0700 Subject: Prevent extra string allocations when no 'rel' arg passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do a check if the 'rel' argument is passed in, and simply set it to 'nofollow' if 'rel' was not passed in. This prevents three string allocations for each call to `link_to` in that scenario. In the scenario where the 'rel' argument is passed in, performance is around the same as before as the `key?` check is very fast. ```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 @hash = {} def master_version "#{@hash['rel'.freeze]} nofollow".lstrip end def fast_version if @hash.key?('rel'.freeze) "#{@hash["rel"]} nofollow".lstrip else "nofollow".freeze end end puts 'no rel key' 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 puts 'rel key' @hash['rel'] = 'hi'.freeze 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 ``` ``` no rel key master_version {:FREE=>-2791, :T_STRING=>3052} fast_version {:FREE=>-1} Warming up -------------------------------------- master_version 80.324k i/100ms fast_version 200.262k i/100ms Calculating ------------------------------------- master_version 2.049M (±11.9%) i/s - 10.121M in 5.025613s fast_version 6.645M (±21.3%) i/s - 29.439M in 5.007488s Comparison: fast_version: 6644506.3 i/s master_version: 2048833.0 i/s - 3.24x slower rel key master_version {:FREE=>-2001, :T_STRING=>2000} fast_version {:FREE=>-2001, :T_STRING=>2000} Warming up -------------------------------------- master_version 155.673k i/100ms fast_version 106.515k i/100ms Calculating ------------------------------------- master_version 2.652M (±20.4%) i/s - 12.610M in 5.036494s fast_version 2.237M (±16.8%) i/s - 10.865M in 5.035366s Comparison: master_version: 2651702.2 i/s fast_version: 2237470.6 i/s - same-ish: difference falls within error ``` --- actionview/lib/action_view/helpers/url_helper.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/actionview/lib/action_view/helpers/url_helper.rb b/actionview/lib/action_view/helpers/url_helper.rb index efda549c0d..4e436433c4 100644 --- a/actionview/lib/action_view/helpers/url_helper.rb +++ b/actionview/lib/action_view/helpers/url_helper.rb @@ -589,10 +589,14 @@ module ActionView end def add_method_to_attributes!(html_options, method) - if method && method.to_s.downcase != "get".freeze && html_options["rel".freeze] !~ /nofollow/ - html_options["rel".freeze] = "#{html_options["rel".freeze]} nofollow".lstrip + if method && method.to_s.downcase != "get" && html_options["rel"] !~ /nofollow/ + if html_options.key?("rel") + html_options["rel"] = "#{html_options["rel"]} nofollow".lstrip + else + html_options["rel"] = "nofollow" + end end - html_options["data-method".freeze] = method + html_options["data-method"] = method end def token_tag(token = nil, form_options: {}) -- cgit v1.2.3 From d404e2cbe9e078d286c7acb13fd18fb3c04a3f4f Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Thu, 2 Nov 2017 14:50:38 -0700 Subject: Use blank? check instead of key? check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows us to prevent an extra string allocation when there is a rel argument and performs better/within error of the key check for other scenarios such as passing in rel: nil ```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 @hash = {} def master_version "#{@hash["rel"]} nofollow".lstrip end def key_version if @hash.key?("rel") "#{@hash["rel"]} nofollow".lstrip else "nofollow" end end def present_version if @hash["rel"].present? "#{@hash["rel"]} nofollow" else "nofollow".freeze end end def nil_version if @hash["rel"].nil? "nofollow".freeze else "#{@hash["rel"]} nofollow" end end def blank_version if @hash["rel"].blank? "nofollow".freeze else "#{@hash["rel"]} nofollow" end end def test puts "master_version" puts allocate_count { 1000.times { master_version } } puts "key_version" puts allocate_count { 1000.times { key_version } } puts "present_version" puts allocate_count { 1000.times { present_version } } puts "nil_version" puts allocate_count { 1000.times { nil_version } } puts "blank_version" puts allocate_count { 1000.times { blank_version } } Benchmark.ips do |x| x.report("master_version") { master_version } x.report("key_version") { key_version } x.report("present_version") { present_version } x.report("nil_version") { nil_version } x.report("blank_version") { blank_version } x.compare! end end puts 'no rel key' test puts 'rel key with real stuff' @hash['rel'] = 'hi'.freeze test puts 'rel key with nil' @hash['rel'] = nil test puts 'rel key with ""' @hash['rel'] = "" test ``` ``` no rel key master_version {:FREE=>-2818, :T_STRING=>3052} key_version {:FREE=>-1} present_version {:FREE=>-1} nil_version {:FREE=>-1} blank_version {:FREE=>-1} Warming up -------------------------------------- master_version 124.677k i/100ms key_version 227.992k i/100ms present_version 208.068k i/100ms nil_version 235.272k i/100ms blank_version 176.274k i/100ms Calculating ------------------------------------- master_version 1.968M (±10.8%) i/s - 9.725M in 5.010763s key_version 7.734M (±11.2%) i/s - 38.075M in 5.001613s present_version 5.688M (±11.4%) i/s - 28.089M in 5.019560s nil_version 6.965M (±10.2%) i/s - 34.585M in 5.024806s blank_version 6.139M (±18.7%) i/s - 29.085M in 5.010919s Comparison: key_version: 7734058.3 i/s nil_version: 6965050.2 i/s - same-ish: difference falls within error blank_version: 6138744.3 i/s - same-ish: difference falls within error present_version: 5688248.4 i/s - 1.36x slower master_version: 1967932.3 i/s - 3.93x slower rel key with real stuff master_version {:FREE=>-2001, :T_STRING=>2000} key_version {:FREE=>-2001, :T_STRING=>2000} present_version {:FREE=>-1001, :T_STRING=>1000} nil_version {:FREE=>-1002, :T_STRING=>1000, :T_IMEMO=>1} blank_version {:FREE=>-1001, :T_STRING=>1000} Warming up -------------------------------------- master_version 93.351k i/100ms key_version 89.747k i/100ms present_version 91.963k i/100ms nil_version 103.370k i/100ms blank_version 74.845k i/100ms Calculating ------------------------------------- master_version 2.179M (±21.4%) i/s - 10.362M in 5.044668s key_version 2.345M (± 9.8%) i/s - 11.667M in 5.030982s present_version 1.738M (±14.8%) i/s - 8.553M in 5.056406s nil_version 2.485M (±19.1%) i/s - 11.888M in 5.015940s blank_version 1.951M (±12.3%) i/s - 9.580M in 5.011932s Comparison: nil_version: 2484704.1 i/s key_version: 2344664.8 i/s - same-ish: difference falls within error master_version: 2178975.8 i/s - same-ish: difference falls within error blank_version: 1950532.0 i/s - same-ish: difference falls within error present_version: 1737866.7 i/s - 1.43x slower rel key with nil master_version {:FREE=>-3001, :T_STRING=>3000} key_version {:FREE=>-3001, :T_STRING=>3000} present_version {:FREE=>-1} nil_version {:FREE=>-1} blank_version {:FREE=>-1} Warming up -------------------------------------- master_version 112.655k i/100ms key_version 105.048k i/100ms present_version 136.219k i/100ms nil_version 192.026k i/100ms blank_version 184.846k i/100ms Calculating ------------------------------------- master_version 1.893M (±12.6%) i/s - 9.238M in 5.002621s key_version 1.672M (±13.5%) i/s - 8.194M in 5.021197s present_version 4.484M (±20.5%) i/s - 21.114M in 5.002982s nil_version 5.294M (±18.1%) i/s - 25.155M in 5.020721s blank_version 5.588M (± 6.7%) i/s - 27.912M in 5.019305s Comparison: blank_version: 5588489.6 i/s nil_version: 5293929.9 i/s - same-ish: difference falls within error present_version: 4484493.7 i/s - same-ish: difference falls within error master_version: 1892919.0 i/s - 2.95x slower key_version: 1672343.9 i/s - 3.34x slower rel key with "" master_version {:FREE=>-2001, :T_STRING=>2000} key_version {:FREE=>-2001, :T_STRING=>2000} present_version {:FREE=>-1} nil_version {:FREE=>-1001, :T_STRING=>1000} blank_version {:FREE=>-1} Warming up -------------------------------------- master_version 140.499k i/100ms key_version 124.738k i/100ms present_version 186.659k i/100ms nil_version 148.063k i/100ms blank_version 178.707k i/100ms Calculating ------------------------------------- master_version 1.826M (±24.2%) i/s - 8.289M in 5.026603s key_version 1.561M (±15.3%) i/s - 7.609M in 5.005662s present_version 3.622M (±19.9%) i/s - 17.173M in 5.042217s nil_version 2.438M (±11.5%) i/s - 12.141M in 5.053335s blank_version 4.911M (±15.5%) i/s - 23.768M in 5.009106s Comparison: blank_version: 4910741.1 i/s present_version: 3622183.5 i/s - same-ish: difference falls within error nil_version: 2437606.2 i/s - 2.01x slower master_version: 1825652.2 i/s - 2.69x slower key_version: 1560530.5 i/s - 3.15x slower ``` --- actionview/lib/action_view/helpers/url_helper.rb | 6 +- test.rb | 110 +++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 test.rb diff --git a/actionview/lib/action_view/helpers/url_helper.rb b/actionview/lib/action_view/helpers/url_helper.rb index 4e436433c4..71fed39351 100644 --- a/actionview/lib/action_view/helpers/url_helper.rb +++ b/actionview/lib/action_view/helpers/url_helper.rb @@ -590,10 +590,10 @@ module ActionView def add_method_to_attributes!(html_options, method) if method && method.to_s.downcase != "get" && html_options["rel"] !~ /nofollow/ - if html_options.key?("rel") - html_options["rel"] = "#{html_options["rel"]} nofollow".lstrip - else + if html_options.("rel").blank? html_options["rel"] = "nofollow" + else + html_options["rel"] = "#{html_options["rel"]} nofollow" end end html_options["data-method"] = method diff --git a/test.rb b/test.rb new file mode 100644 index 0000000000..3d5e508fa7 --- /dev/null +++ b/test.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true +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 + +@hash = {} + +def master_version + "#{@hash["rel"]} nofollow".lstrip +end + +def key_version + if @hash.key?("rel") + "#{@hash["rel"]} nofollow".lstrip + else + "nofollow" + end +end + +def present_version + if @hash["rel"].present? + "#{@hash["rel"]} nofollow" + else + "nofollow".freeze + end +end + +def nil_version + if @hash["rel"].nil? + "nofollow".freeze + else + "#{@hash["rel"]} nofollow" + end +end + +def blank_version + if @hash["rel"].blank? + "nofollow".freeze + else + "#{@hash["rel"]} nofollow" + end +end + +def test + puts "master_version" + puts allocate_count { 1000.times { master_version } } + puts "key_version" + puts allocate_count { 1000.times { key_version } } + puts "present_version" + puts allocate_count { 1000.times { present_version } } + puts "nil_version" + puts allocate_count { 1000.times { nil_version } } + puts "blank_version" + puts allocate_count { 1000.times { blank_version } } + + Benchmark.ips do |x| + x.report("master_version") { master_version } + x.report("key_version") { key_version } + x.report("present_version") { present_version } + x.report("nil_version") { nil_version } + x.report("blank_version") { blank_version } + x.compare! + end +end + +puts 'no rel key' + +test + +puts 'rel key with real stuff' + +@hash['rel'] = 'hi'.freeze + +test + +puts 'rel key with nil' + +@hash['rel'] = nil + +test + +puts 'rel key with ""' + +@hash['rel'] = "" + +test -- cgit v1.2.3 From 019c8ae814d0e89af3da543a956f22a4db92c5a3 Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Thu, 2 Nov 2017 14:55:51 -0700 Subject: Remove test file --- test.rb | 110 ---------------------------------------------------------------- 1 file changed, 110 deletions(-) delete mode 100644 test.rb diff --git a/test.rb b/test.rb deleted file mode 100644 index 3d5e508fa7..0000000000 --- a/test.rb +++ /dev/null @@ -1,110 +0,0 @@ -# frozen_string_literal: true -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 - -@hash = {} - -def master_version - "#{@hash["rel"]} nofollow".lstrip -end - -def key_version - if @hash.key?("rel") - "#{@hash["rel"]} nofollow".lstrip - else - "nofollow" - end -end - -def present_version - if @hash["rel"].present? - "#{@hash["rel"]} nofollow" - else - "nofollow".freeze - end -end - -def nil_version - if @hash["rel"].nil? - "nofollow".freeze - else - "#{@hash["rel"]} nofollow" - end -end - -def blank_version - if @hash["rel"].blank? - "nofollow".freeze - else - "#{@hash["rel"]} nofollow" - end -end - -def test - puts "master_version" - puts allocate_count { 1000.times { master_version } } - puts "key_version" - puts allocate_count { 1000.times { key_version } } - puts "present_version" - puts allocate_count { 1000.times { present_version } } - puts "nil_version" - puts allocate_count { 1000.times { nil_version } } - puts "blank_version" - puts allocate_count { 1000.times { blank_version } } - - Benchmark.ips do |x| - x.report("master_version") { master_version } - x.report("key_version") { key_version } - x.report("present_version") { present_version } - x.report("nil_version") { nil_version } - x.report("blank_version") { blank_version } - x.compare! - end -end - -puts 'no rel key' - -test - -puts 'rel key with real stuff' - -@hash['rel'] = 'hi'.freeze - -test - -puts 'rel key with nil' - -@hash['rel'] = nil - -test - -puts 'rel key with ""' - -@hash['rel'] = "" - -test -- cgit v1.2.3 From ec13ef75260f6d143e947b0e5176e35e4c1229c2 Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Thu, 2 Nov 2017 17:54:54 -0700 Subject: Fix typo --- actionview/lib/action_view/helpers/url_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionview/lib/action_view/helpers/url_helper.rb b/actionview/lib/action_view/helpers/url_helper.rb index 71fed39351..9900e0cd03 100644 --- a/actionview/lib/action_view/helpers/url_helper.rb +++ b/actionview/lib/action_view/helpers/url_helper.rb @@ -590,7 +590,7 @@ module ActionView def add_method_to_attributes!(html_options, method) if method && method.to_s.downcase != "get" && html_options["rel"] !~ /nofollow/ - if html_options.("rel").blank? + if html_options["rel"].blank? html_options["rel"] = "nofollow" else html_options["rel"] = "#{html_options["rel"]} nofollow" -- cgit v1.2.3