Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer 'Regexp#match?' over 'Regexp#=~' when the result is not needed #98

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Prefer 'Regexp#match?' over 'Regexp#=~' when the result is not needed #98

merged 1 commit into from
Feb 11, 2021

Conversation

jamescook
Copy link
Contributor

Problem

The ValidMimeType middleware is allocating Matchdata objects but does not need them. Since the middleware runs on every request, I think it's worthwhile to optimize.

Solution

Use Regexp#match? instead.

Here's a benchmark script:

# Usage: ruby regexp_bm.rb

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'benchmark-memory'
end

# JSON
accept = "application/json"

Benchmark.ips do |b|
	b.time = 15
  b.report("=~ for json") do
    (accept =~ %r{application/json} && accept =~ %r{version=\d+}) || accept =~ %r{application/protobuf}
  end

  b.report(".match? for json") do
    (%r{application/json}.match?(accept) && %r{version=\d+}.match?(accept)) || %r{application/protobuf}.match?(accept)
  end

  b.compare!
end

# PROTOBUF
accept = "application/protobuf"
Benchmark.ips do |b|
	b.time = 15
  b.report("=~ for protobuf") do
    (accept =~ %r{application/json} && accept =~ %r{version=\d+}) || accept =~ %r{application/protobuf}
  end

  b.report(".match? for protobuf") do
    (%r{application/json}.match?(accept) && %r{version=\d+}.match?(accept)) || %r{application/protobuf}.match?(accept)
  end

  b.compare!
end

Benchmark.memory do |b|
  b.report("=~") do
    (accept =~ %r{application/json} && accept =~ %r{version=\d+}) || accept =~ %r{application/protobuf}
  end

  b.report(".match?") do
    (%r{application/json}.match?(accept) && %r{version=\d+}.match?(accept)) || %r{application/protobuf}.match?(accept)
  end

  b.compare!
end

Benchmark results on my machine:

07:06 $ ruby -v
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin19]

07:06 $ ruby regexp_bm.rb
Warming up --------------------------------------
         =~ for json    72.038k i/100ms
    .match? for json   328.950k i/100ms
Calculating -------------------------------------
         =~ for json    751.188k (± 1.5%) i/s -     11.310M in  15.059465s
    .match? for json      3.279M (± 1.0%) i/s -     49.342M in  15.050594s

Comparison:
    .match? for json:  3278753.1 i/s
         =~ for json:   751188.5 i/s - 4.36x  (± 0.00) slower

Warming up --------------------------------------
     =~ for protobuf    92.923k i/100ms
.match? for protobuf   398.059k i/100ms
Calculating -------------------------------------
     =~ for protobuf    935.759k (± 2.0%) i/s -     14.031M in  15.000566s
.match? for protobuf      3.975M (± 1.1%) i/s -     59.709M in  15.022371s

Comparison:
.match? for protobuf:  3975149.0 i/s
     =~ for protobuf:   935759.2 i/s - 4.25x  (± 0.00) slower

Calculating -------------------------------------
                  =~   208.000  memsize (   208.000  retained)
                         2.000  objects (     2.000  retained)
                         1.000  strings (     1.000  retained)
             .match?     0.000  memsize (     0.000  retained)
                         0.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
             .match?:          0 allocated
                  =~:        208 allocated - Infx more

'=~' allocates a MatchData, 'match?' does not
@jamescook jamescook requested review from rohitbordia and removed request for a team February 7, 2021 13:18
Copy link
Contributor

@brettfishman brettfishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamescook thanks for this contribution! Looks like a nice boost in perf and much more efficient memory use. 👍

@brettfishman brettfishman merged commit 6d5fe27 into stitchfix:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants