Optimise performance of String#blank? #9958

Closed
wants to merge 1 commit into
from

5 participants

@SamSaffron
require "rubygems"
require "benchmark"

n = 100000

class String
  def blank?
    self !~ /[^[:space:]]/
  end

  def blank2?
    self =~ /^[[:space:]*]$/m?true:false
  end

end

tests = {
:s1 => "",
:s2 => "   \r \n",
:s3 => "hello",
:s4 => "   hello"
}

Benchmark.bmbm {|x|
  tests.each do |k,v|
    x.report("blank #{k}") {n.times { v.blank? } }
    x.report("blank2 #{k}") {n.times { v.blank2? } }
  end
}
                user     system      total        real
blank s1    0.040000   0.000000   0.040000 (  0.042660)
blank2 s1   0.020000   0.000000   0.020000 (  0.030270)
blank s2    0.060000   0.000000   0.060000 (  0.055076)
blank2 s2   0.050000   0.000000   0.050000 (  0.047653)
blank s3    0.080000   0.000000   0.080000 (  0.079468)
blank2 s3   0.040000   0.000000   0.040000 (  0.040230)
blank s4    0.090000   0.000000   0.090000 (  0.085941)
blank2 s4   0.040000   0.000000   0.040000 (  0.043885)

Similar results on both 1.9.3 and 2.0.0


Picked this up while looking at a flame graph on Discourse"

image

See: http://samsaffron.com/archive/2013/03/19/flame-graphs-in-ruby-miniprofiler about flame graphs


Also, I wonder if value_to_boolean in AR should even be calling blank?, shouldn't .nil? || .empty? do ... it would be much faster

@steveklabnik
Ruby on Rails member

Seems legit.

@mrb

👍 nice work @SamSaffron

@tenderlove
Ruby on Rails member

No, it doesn't seem legit:

class String
  def blank?
    self !~ /[^[:space:]]/
  end

  def blank2?
    self =~ /^[[:space:]*]$/m?true:false
  end

end

p "  ".blank?  # => true
p "  ".blank2? # => false
@SamSaffron
@tenderlove
Ruby on Rails member

Also, I wonder if value_to_boolean in AR should even be calling blank?, shouldn't .nil? || .empty? do ... it would be much faster

@SamSaffron I would vote for this rather than muck with blank?

@SamSaffron

@tenderlove yeah ... once I corrected the regex this becomes a bit slower ...

will close ...

just to confirm ... do you know of any dbs that return " " and really mean null ?

@SamSaffron SamSaffron closed this Mar 27, 2013
@SamSaffron

FYI, moved discussion here: 111611b#commitcomment-2894924

its the proper way to fix this anyway, blank should not be being called tons of times from the internals.

@SamSaffron SamSaffron referenced this pull request Mar 27, 2013
@carlosantoniodasilva carlosantoniodasilva Revert "Merge pull request #9784 from vipulnsward/change_from_blank_t…
…o_empty_on_string"

This reverts commit 9c4c05f, reversing
changes made to 4620bdc.

Reason: They're not completely interchangeable, since blank? will also
check for strings containing spaces.
111611b
@vais

@SamSaffron @tenderlove Do you think this work-around is an acceptable compromise/interim optimization until the problem with String#strip not working with unicode gets fixed in ruby as documented in this thread?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment