Skip to content

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Apr 27, 2025

Which issue(s) this PR fixes:
Fixes #

What this PR does / why we need it:

This is cosmetic change, it does not change behavior at all. The following rubocop configuration detects it.

Performance/ConstantRegexp:
  Enable: true

Benchmark result:

ruby 3.2.8 (2025-03-26 revision 13f495dc2c) +YJIT [x86_64-linux]
Warming up --------------------------------------
 interpret everytime    26.127k i/100ms
      interpret once     3.172M i/100ms
Calculating -------------------------------------
 interpret everytime    254.171k (± 2.6%) i/s    (3.93 μs/i) -      1.280M in   5.040357s
      interpret once     33.755M (± 1.7%) i/s   (29.62 ns/i) -    171.289M in   5.075867s

Comparison:
      interpret once: 33755320.3 i/s
 interpret everytime:   254171.5 i/s - 132.81x  slower

Regex should not be evaluated every time, use /o to evaluate only once because it expands constant in /#{...}/ expression.

Appendix: benchmark script

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

Benchmark.ips do |x|
  ZERO_OR_MORE_SPACING = /(?:[ \t\r\n]|\z|\#.*?(?:\z|[\r\n]))*/
  x.report("interpret everytime") {
    /(?:#{ZERO_OR_MORE_SPACING}\>)/
  }
  x.report("interpret once") {
    /(?:#{ZERO_OR_MORE_SPACING}\>)/o
  }
  x.compare!
end

Docs Changes:

N/A

Release Note:

N/A

This is cosmetic change, it does not change behavior at all. The following rubocop configuration detects it.

```
Performance/ConstantRegexp:
  Enable: true
```

Benchmark result:

```
ruby 3.2.8 (2025-03-26 revision 13f495dc2c) +YJIT [x86_64-linux]
Warming up --------------------------------------
 interpret everytime    26.127k i/100ms
      interpret once     3.172M i/100ms
Calculating -------------------------------------
 interpret everytime    254.171k (± 2.6%) i/s    (3.93 μs/i) -      1.280M in   5.040357s
      interpret once     33.755M (± 1.7%) i/s   (29.62 ns/i) -    171.289M in   5.075867s

Comparison:
      interpret once: 33755320.3 i/s
 interpret everytime:   254171.5 i/s - 132.81x  slower
```

Regex should not be evaluated every time, use /o to evaluate only
once because it expands constant in /#{...}/ expression.

Appendix: benchmark script

```ruby
require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'benchmark-ips'
  gem 'benchmark-memory'
end

Benchmark.ips do |x|
  ZERO_OR_MORE_SPACING = /(?:[ \t\r\n]|\z|\#.*?(?:\z|[\r\n]))*/
  x.report("interpret everytime") {
    /(?:#{ZERO_OR_MORE_SPACING}\>)/
  }
  x.report("interpret once") {
    /(?:#{ZERO_OR_MORE_SPACING}\>)/o
  }
  x.compare!
end
```

Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
@kenhys kenhys requested review from Watson1978 and daipom April 28, 2025 04:04
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks!

@daipom daipom merged commit 8a4959f into fluent:master Apr 28, 2025
18 of 19 checks passed
@kenhys kenhys deleted the rubocop-constant-regexp branch April 28, 2025 04:58
@daipom daipom added this to the v1.19.0 milestone Apr 30, 2025
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.

3 participants