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

Add IOSource#match? method #216

Merged
merged 2 commits into from
Oct 27, 2024
Merged

Add IOSource#match? method #216

merged 2 commits into from
Oct 27, 2024

Conversation

naitoh
Copy link
Contributor

@naitoh naitoh commented Oct 21, 2024

Why?

StringScanner#match? is faster than StringScanner#check.

See: ruby/strscan#111

Benchmark

RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.4/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml
ruby 3.3.4 (2024-07-09 revision be1089c8ec) [arm64-darwin22]
Calculating -------------------------------------
                         before       after  before(YJIT)  after(YJIT)
                 dom     18.819      19.362        32.846       34.708 i/s -     100.000 times in 5.313905s 5.164791s 3.044500s 2.881200s
                 sax     28.188      29.982        48.386       52.554 i/s -     100.000 times in 3.547597s 3.335304s 2.066732s 1.902809s
                pull     31.962      33.902        57.868       60.662 i/s -     100.000 times in 3.128689s 2.949690s 1.728071s 1.648467s
              stream     31.436      33.030        52.808       56.647 i/s -     100.000 times in 3.181095s 3.027574s 1.893635s 1.765304s

Comparison:
                              dom
         after(YJIT):        34.7 i/s
        before(YJIT):        32.8 i/s - 1.06x  slower
               after:        19.4 i/s - 1.79x  slower
              before:        18.8 i/s - 1.84x  slower

                              sax
         after(YJIT):        52.6 i/s
        before(YJIT):        48.4 i/s - 1.09x  slower
               after:        30.0 i/s - 1.75x  slower
              before:        28.2 i/s - 1.86x  slower

                             pull
         after(YJIT):        60.7 i/s
        before(YJIT):        57.9 i/s - 1.05x  slower
               after:        33.9 i/s - 1.79x  slower
              before:        32.0 i/s - 1.90x  slower

                           stream
         after(YJIT):        56.6 i/s
        before(YJIT):        52.8 i/s - 1.07x  slower
               after:        33.0 i/s - 1.72x  slower
              before:        31.4 i/s - 1.80x  slower

  • YJIT=ON : 1.05x - 1.09x faster
  • YJIT=OFF : 1.02x - 1.06x faster

@naitoh naitoh marked this pull request as ready for review October 21, 2024 13:05
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

In general, this is a good improvement.
But can we use better name than skip? skip("...") doesn't skip.

lib/rexml/source.rb Outdated Show resolved Hide resolved
@naitoh naitoh changed the title Add IOSource#skip() method Add IOSource#match_size() method Oct 26, 2024
@naitoh
Copy link
Contributor Author

naitoh commented Oct 26, 2024

But can we use better name than skip? skip("...") doesn't skip.

I see.
I changed the method name to match_size().

@naitoh naitoh requested a review from kou October 26, 2024 14:59
@kou
Copy link
Member

kou commented Oct 26, 2024

It seems that all callers don't use the returned size. They just check whether the return value is nil or not.

Is size in match_size an important information? Can we use just a boolean instead of the matched size as a return value instead?

naitoh and others added 2 commits October 27, 2024 07:09
## Why?
`StringScanner#match?()` is faster than `StringScanner#check()`.

See: ruby/strscan#111

## Benchmark
```
RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.4/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml
ruby 3.3.4 (2024-07-09 revision be1089c8ec) [arm64-darwin22]
Calculating -------------------------------------
                         before       after  before(YJIT)  after(YJIT)
                 dom     18.819      19.362        32.846       34.708 i/s -     100.000 times in 5.313905s 5.164791s 3.044500s 2.881200s
                 sax     28.188      29.982        48.386       52.554 i/s -     100.000 times in 3.547597s 3.335304s 2.066732s 1.902809s
                pull     31.962      33.902        57.868       60.662 i/s -     100.000 times in 3.128689s 2.949690s 1.728071s 1.648467s
              stream     31.436      33.030        52.808       56.647 i/s -     100.000 times in 3.181095s 3.027574s 1.893635s 1.765304s

Comparison:
                              dom
         after(YJIT):        34.7 i/s
        before(YJIT):        32.8 i/s - 1.06x  slower
               after:        19.4 i/s - 1.79x  slower
              before:        18.8 i/s - 1.84x  slower

                              sax
         after(YJIT):        52.6 i/s
        before(YJIT):        48.4 i/s - 1.09x  slower
               after:        30.0 i/s - 1.75x  slower
              before:        28.2 i/s - 1.86x  slower

                             pull
         after(YJIT):        60.7 i/s
        before(YJIT):        57.9 i/s - 1.05x  slower
               after:        33.9 i/s - 1.79x  slower
              before:        32.0 i/s - 1.90x  slower

                           stream
         after(YJIT):        56.6 i/s
        before(YJIT):        52.8 i/s - 1.07x  slower
               after:        33.0 i/s - 1.72x  slower
              before:        31.4 i/s - 1.80x  slower

```

- YJIT=ON : 1.06x - 1.09x faster
- YJIT=OFF : 1.02x - 1.06x faster

Co-authored-by: Sutou Kouhei <kou@clear-code.com>
@naitoh
Copy link
Contributor Author

naitoh commented Oct 26, 2024

Is size in match_size an important information?

No.

Can we use just a boolean instead of the matched size as a return value instead?

OK.
I changed the method name to match?().

@naitoh naitoh changed the title Add IOSource#match_size() method Add IOSource#match?() method Oct 26, 2024
@kou kou changed the title Add IOSource#match?() method Add IOSource#match? method Oct 27, 2024
@kou
Copy link
Member

kou commented Oct 27, 2024

Thanks.
match? is better than skip and match_size.

@kou kou merged commit 8ef7502 into ruby:master Oct 27, 2024
61 checks passed
@naitoh naitoh deleted the skip branch October 27, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants