Don't instantiate or use Regexp when string.start_with? will do

[mathias]

What are you trying to accomplish?

While doing some unrelated profiling work in GitHub, I noticed infer_selector_key coming up often. I looked at the source because I was curious, and saw it was using this Hash of a bunch of Regexp. At first I though the goal in infer_selector_key was to gsub (replace) the contents of selector, but it looks like it just returns the value of the hash if it matches. Regexp instances have a little bit more overhead than Strings, especially when String's start_with? is written all in C.

Generally, we don't need to wrap things in Regexp unless we want to do something complex with capture groups, etc., in Ruby.

Before making a change, I wrote a benchmark using benchmark-ips (which figures out how many Iterations-Per-Second the blocks of code can do, on its own, so I didn't have to pick statistically-significant iteration counts) and this script just isolates out the logic of the change we want to use --

require "benchmark/ips"

REPLACEMENT_KEYS = {
  # NOTE: Copied from `lib/primer/classify/utilities.r`
  # Elided here for brevity
}.freeze

Benchmark.ips do |x|
  x.report("regexp match - case matches") do
    "rounded_border".match?(REPLACEMENT_KEYS.keys.last)
  end

  x.report("regexp match - case does not match") do
    "font".match?(REPLACEMENT_KEYS.keys.last)
  end

  x.report("string starts_with - case matches") do
    "rounded_border".start_with?("rounded")
  end

  x.report("string starts_with - case does not match") do
    "font".start_with?("rounded")
  end

  x.compare!
end

And the outputs:

$ safe-ruby script/benchmark-matt.rb
ruby 3.3.5 (2024-09-04 revision 4f143c3038) [x86_64-linux]
Warming up --------------------------------------
regexp match - case matches
                       382.111k i/100ms
regexp match - case does not match
                       480.783k i/100ms
string starts_with - case matches
                       613.842k i/100ms
string starts_with - case does not match
                       663.965k i/100ms
Calculating -------------------------------------
regexp match - case matches
                          3.887M (± 2.2%) i/s -     19.488M in   5.015503s
regexp match - case does not match
                          4.770M (± 1.7%) i/s -     24.039M in   5.040564s
string starts_with - case matches
                          6.336M (± 1.7%) i/s -     31.920M in   5.039189s
string starts_with - case does not match
                          6.641M (± 2.0%) i/s -     33.862M in   5.100539s

Comparison:
string starts_with - case does not match:  6641407.3 i/s
string starts_with - case matches:  6336044.9 i/s - 1.05x  slower
regexp match - case does not match:  4770427.1 i/s - 1.39x  slower
regexp match - case matches:  3887307.8 i/s - 1.71x  slower

As you can see, switching to a string and using start_with is faster when we isolate out looping over the Hash, etc.

On the code I was looking at, the start_with? version getting called 1306 times should result in roughly a 3ms faster execution. Not huge, but since I could see this improvement, I made it.

The only place where Regexp.new was actually needed as in lib/tasks/utilities.rake where we do a sub, but it was also essentially wrapping the keys and therefore calling Regexp.new(Regexp.new("^f")) (or whatever the value inside was. The rake task probably doesn't get called as much as the class is used, but we should still benefit here.

Integration

No, this should not change any usage of infer_selector_key.

List the issues that this change affects.

Closes #3176

Risk Assessment

• Low risk the change is small, highly observable, and easily rolled back.

What approach did you choose and why?

See above with benchmarks.

Anything you want to highlight for special attention from reviewers?

Accessibility

N/A should not change functionality so should not affect accessibility

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: e2072fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mathias]

I should add that I looked for other calls to Regexp.new but didn't see anything else that stuck out.

[mathias]

Changeset added -- chose patch given that it should not change functionality. Please advise if I need to correct it.

[camertron]

Awesome, thank you! 🎉