Avoid unneccessarily converting nil to string

[bquorning]

Running the current ActiveRecord tests, I saw #sanitize_sql_for_order being called 20068 times with an empty array, and only 2805 times with a non-empty array.

When called with an empty array, the first line ([].first.to_s.include("?")) will unneccessarily allocate two strings, "" and "?". By freezing the "?" and checking against empty arrays, we can make the method more than twice as fast in the most common case. For other arguments, the runtime is comparable to the current implementation.

Performance script:

require 'benchmark/ips'

Benchmark.ips do |x|
  condition = ["name=? and group_id=?", "foo'bar", 4]
  x.report("original")    { condition.is_a?(Array) && condition.first.to_s.include?("?") }
  x.report("empty-check") { condition.is_a?(Array) && condition.any? && condition.first.to_s.include?("?".freeze) }
  x.compare!
end

Benchmark.ips do |x|
  condition = ["position"]
  x.report("original")    { condition.is_a?(Array) && condition.first.to_s.include?("?") }
  x.report("empty-check") { condition.is_a?(Array) && condition.any? && condition.first.to_s.include?("?".freeze) }
  x.compare!
end

Benchmark.ips do |x|
  condition = []
  x.report("original")    { condition.is_a?(Array) && condition.first.to_s.include?("?") }
  x.report("empty-check") { condition.is_a?(Array) && condition.any? && condition.first.to_s.include?("?".freeze) }
  x.compare!
end

Results:

Warming up --------------------------------------
            original   200.685k i/100ms
         empty-check   182.827k i/100ms
Calculating -------------------------------------
            original      4.169M (±10.3%) i/s -     20.671M in   5.040107s
         empty-check      4.223M (± 2.8%) i/s -     21.208M in   5.026017s

Comparison:
         empty-check:  4222988.3 i/s
            original:  4169126.2 i/s - same-ish: difference falls within error

Warming up --------------------------------------
            original   209.549k i/100ms
         empty-check   212.640k i/100ms
Calculating -------------------------------------
            original      4.304M (± 6.1%) i/s -     21.584M in   5.035971s
         empty-check      4.345M (± 2.7%) i/s -     21.902M in   5.044407s

Comparison:
         empty-check:  4345159.0 i/s
            original:  4303567.5 i/s - same-ish: difference falls within error

Warming up --------------------------------------
            original   191.417k i/100ms
         empty-check   278.279k i/100ms
Calculating -------------------------------------
            original      3.578M (± 6.9%) i/s -     17.802M in   5.009026s
         empty-check      7.684M (± 6.2%) i/s -     38.403M in   5.032166s

Comparison:
         empty-check:  7684342.8 i/s
            original:  3578112.8 i/s - 2.15x  slower

See also #22133.
cc/ @yui-knk
r? @pixeltrix

[matthewd]

Constant-per-query costs aren't really high value optimization targets: by your numbers, with 20000 calls, this gains 3ms in a 3.5 minute test run.

But more notable than the absolute gain, is the relative one: this method is generally called when we're in the process of assembling a query, ready to send it to the database server, wait while it executes, and then create a huge number of objects to represent the result. The "1000 papercuts" approach to improving performance is not unreasonable... but I think we'll need to look in deeper loops to see worthwhile gains.

(Should we perhaps instead be looking to make the common case involve a nil instead of []? Or does that just create more work elsewhere?)

---

If we're going to do this, !empty? is measurably faster than any?. But I'm reluctant to make a change that won't show up on any benchmark, and will just get refactored out as redundant next time someone touches the method. And this gain certainly doesn't feel like it warrants a "do not touch this because performance" comment for protection.