From d404e2cbe9e078d286c7acb13fd18fb3c04a3f4f Mon Sep 17 00:00:00 2001 From: Dillon Welch Date: Thu, 2 Nov 2017 14:50:38 -0700 Subject: [PATCH] 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 ``` --- .../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 @@ def link_to_remote_options?(options) 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 -- GitLab