Skip to content

Rewrite Integer#times in Ruby#8388

Merged
k0kubun merged 1 commit intoruby:masterfrom
k0kubun:builtin-times
Sep 7, 2023
Merged

Rewrite Integer#times in Ruby#8388
k0kubun merged 1 commit intoruby:masterfrom
k0kubun:builtin-times

Conversation

@k0kubun
Copy link
Copy Markdown
Member

@k0kubun k0kubun commented Sep 6, 2023 •
edited
Loading

railsbench

I benchmarked this with railsbench, which does call Integer#times a little (ruby/ruby-bench#168).

Interpreter

It doesn't seem to have a significant impact on the interpreter.

before: ruby 3.3.0dev (2023-09-06T19:50:21Z master acd626a583) [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T22:03:28Z builtin-times 74e1bd7243) [x86_64-linux]

----------  -----------  ----------  ----------  ----------  -------------  ------------
bench       before (ms)  stddev (%)  after (ms)  stddev (%)  after 1st itr  before/after
railsbench  2018.9       0.8         2015.0      0.9         1.01           1.00
----------  -----------  ----------  ----------  ----------  -------------  ------------

YJIT

It decreases interp_return, increases avg_len_in_yjit, and speeds up the benchmark a little.

before: ruby 3.3.0dev (2023-09-06T19:50:21Z master acd626a583) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T22:03:28Z builtin-times 74e1bd7243) +YJIT [x86_64-linux]

----------  -----------  ----------  ----------  ----------  -------------  ------------
bench       before (ms)  stddev (%)  after (ms)  stddev (%)  after 1st itr  before/after
railsbench  1155.4       1.4         1137.3      1.3         1.01           1.02
----------  -----------  ----------  ----------  ----------  -------------  ------------

microbenchmark

Interpreter

It slows down the interpreter a little, but it doesn't seem too significant. In fact, railsbench did not slow down.

$ benchmark-driver benchmark/loop_times.rb -v --chruby 'before;after'
before: ruby 3.3.0dev (2023-09-06T19:50:21Z master acd626a583) [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T22:03:28Z builtin-times 74e1bd7243) [x86_64-linux]
Calculating -------------------------------------
                         before       after
          loop_times      1.290       1.071 i/s -       1.000 times in 0.774922s 0.933629s

Comparison:
                       loop_times
              before:         1.3 i/s
               after:         1.1 i/s - 1.20x  slower

YJIT

Ruby outperforms C.

$ benchmark-driver benchmark/loop_times.rb --chruby 'before::before --yjit-call-threshold=1;after::after --yjit-call-threshold=1' -v
before: ruby 3.3.0dev (2023-09-06T19:50:21Z master acd626a583) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T22:03:28Z builtin-times 74e1bd7243) +YJIT [x86_64-linux]
Calculating -------------------------------------
                         before       after
          loop_times      1.490       5.229 i/s -       1.000 times in 0.670956s 0.191239s

Comparison:
                       loop_times
               after:         5.2 i/s
              before:         1.5 i/s - 3.51x  slower

@k0kubun k0kubun marked this pull request as ready for review September 6, 2023 22:20
paracycle
paracycle reviewed Sep 6, 2023
View reviewed changes
Comment thread numeric.rb Outdated
nobu
nobu reviewed Sep 7, 2023
View reviewed changes
Comment thread numeric.rb
@casperisfine
Copy link
Copy Markdown
Contributor

casperisfine commented Sep 7, 2023

In fact, railsbench did not slow down.

I wouldn't expect Integer#times to be called much in railsbench. But in general I wouldn't expect it to be called much in hotspots at all.

Array#each would be the big one.

@maximecb
Copy link
Copy Markdown
Contributor

maximecb commented Sep 7, 2023

This is pretty cool. Can you run it on rubykon, mail, nbody, optcarrot and lee to make sure there's no degradation? I expect it will perform well, but it would be nice to have a bit more benchmark data.

@k0kubun
Copy link
Copy Markdown
Member Author

k0kubun commented Sep 7, 2023

Array#each would be the big one.

Benoit thinks rewriting Array#each in Ruby makes it thread-unsafe #6687 (comment), so we're currently blocked by that.

Can you run it on rubykon, mail, nbody, optcarrot and lee to make sure there's no degradation?

Sure

Interpreter

before: ruby 3.3.0dev (2023-09-06T22:26:11Z master 54274b8c65) [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T23:26:33Z builtin-times a2bee28ecc) [x86_64-linux]

---------  -----------  ----------  ----------  ----------  -------------  ------------
bench      before (ms)  stddev (%)  after (ms)  stddev (%)  after 1st itr  before/after
mail       121.1        0.2         121.1       0.2         1.00           1.00
lee        926.8        0.8         938.0       0.7         0.99           0.99
nbody      93.0         1.1         85.5        0.6         1.08           1.09
optcarrot  4525.8       0.7         4544.9      0.6         1.01           1.00
rubykon    10239.2      0.5         10140.2     0.6         1.01           1.01
---------  -----------  ----------  ----------  ----------  -------------  ------------

YJIT

rubykon also seems to be sped up reliably.

before: ruby 3.3.0dev (2023-09-06T22:26:11Z master 54274b8c65) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-09-06T23:26:33Z builtin-times a2bee28ecc) +YJIT [x86_64-linux]

---------  -----------  ----------  ----------  ----------  -------------  ------------
bench      before (ms)  stddev (%)  after (ms)  stddev (%)  after 1st itr  before/after
mail       83.2         0.2         83.6        0.2         1.00           1.00
lee        619.5        1.1         626.9       1.1         0.99           0.99
nbody      41.4         0.5         41.3        0.3         1.00           1.00
optcarrot  1236.9       0.8         1241.2      0.5         1.01           1.00
rubykon    4353.5       0.4         4253.8      0.9         1.02           1.02
---------  -----------  ----------  ----------  ----------  -------------  ------------

@k0kubun k0kubun merged commit 5b5ae3d into ruby:master Sep 7, 2023
@k0kubun k0kubun deleted the builtin-times branch September 7, 2023 17:57
@k0kubun k0kubun mentioned this pull request Sep 8, 2023
Merged
eightbitraptor added a commit to eightbitraptor/ruby that referenced this pull request Sep 15, 2023
@eightbitraptor
Revert "Rewrite Integer#times in Ruby (ruby#8388)"
224101c 
This reverts commit 5b5ae3d.
eightbitraptor added a commit to eightbitraptor/ruby that referenced this pull request Sep 15, 2023
@eightbitraptor
Revert "Revert "Rewrite Integer#times in Ruby (ruby#8388)""
b539609 
This reverts commit 684cf0f3bcdbac32c3457fee7fcedc76e0cfa329.
@k0kubun k0kubun mentioned this pull request Jan 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nobu nobu nobu left review comments

@maximecb maximecb maximecb approved these changes

+1 more reviewer

@paracycle paracycle paracycle left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@k0kubun @casperisfine @maximecb @nobu @paracycle