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

Use size_t for RSTRING_LEN in String#count #4001

Merged
merged 1 commit into from Dec 25, 2020

Conversation

nobu
Copy link
Member

@nobu nobu commented Dec 25, 2020

@ioquatix
Copy link
Member

Would it make sense to standardise on size_t for this kind of use case?

@nobu nobu force-pushed the h1-1042722-String#count-overflow branch from e347a97 to 58020c2 Compare December 25, 2020 12:58
@nobu nobu changed the title Use long for RSTRING_LEN in String#count Use size_t for RSTRING_LEN in String#count Dec 25, 2020
@nobu nobu force-pushed the h1-1042722-String#count-overflow branch from 58020c2 to e889554 Compare December 25, 2020 13:24
@nobu nobu merged commit 6083fed into ruby:master Dec 25, 2020
@nobu nobu deleted the h1-1042722-String#count-overflow branch December 26, 2020 00:45
@ilyazub
Copy link
Contributor

ilyazub commented Sep 11, 2023

String#count became 3.4 times slower after this PR: https://www.reddit.com/r/ruby/comments/16d4ha6/comment/k04ohqu/?utm_source=share&utm_medium=web2x&context=3

Benchmark 1: ./ruby test.rb
  Time (mean ± σ):      95.5 ms ±   1.2 ms    [User: 93.9 ms, System: 1.9 ms]
  Range (min … max):    93.4 ms …  98.5 ms    30 runs

Benchmark 2: ./ruby.master test.rb
  Time (mean ± σ):     326.5 ms ±   2.0 ms    [User: 323.1 ms, System: 3.6 ms]
  Range (min … max):   324.2 ms … 329.1 ms    10 runs

Summary
  './ruby test.rb' ran
    3.42 ± 0.05 times faster than './ruby.master test.rb'

@Freaky
Copy link
Contributor

Freaky commented Sep 12, 2023

Slightly-tweaked test.rb that demonstrates the difference just as well:

html = IO.read('/dev/zero', 1024 * 1024)
Array.new(1000) { html.count($/) }

AMD 5700X, FreeBSD 13.2 (clang 14.0.5), tested against b1f0d00 and same with this patch reverted.

@ilyazub
Copy link
Contributor

ilyazub commented Sep 12, 2023

Here're the benchmark results using the benchmark-driver gem.

$ benchmark-driver tmp/string_count_benchmark_driver.yml --rbenv '3.1.1;3.1.4;2.7.2;3.2.2;3.0.6'                                                 
Calculating -------------------------------------
                          3.1.1       3.1.4       2.7.2       3.2.2       3.0.6
               count    465.804     463.741     865.783     462.711     857.395 i/s -     10.000k times in 21.468251s 21.563768s 11.550239s 21.611783s 11.663235s

Comparison:
                            count
               2.7.2:       865.8 i/s 
               3.0.6:       857.4 i/s - 1.01x  slower
               3.1.1:       465.8 i/s - 1.86x  slower
               3.1.4:       463.7 i/s - 1.87x  slower
               3.2.2:       462.7 i/s - 1.87x  slower

Benchmark:

# cat tmp/string_count_benchmark_driver.yml
loop_count: 10_000
prelude: |
  html = IO.read('/dev/zero', 1024 * 1024)
benchmark:
  count: html.count($/)

i7-6700HQ CPU @ 2.60GHz, Debian 10 (gcc 8.3.0)

@ilyazub
Copy link
Contributor

ilyazub commented Sep 12, 2023

I created a Redmine ticket: https://bugs.ruby-lang.org/issues/19875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants