Skip to content

Rewrite Array#each in Ruby#6687

Closed
k0kubun wants to merge 2 commits intoruby:masterfrom
Shopify:builtin-array-each
Closed

Rewrite Array#each in Ruby#6687
k0kubun wants to merge 2 commits intoruby:masterfrom
Shopify:builtin-array-each

Conversation

@k0kubun
Copy link
Copy Markdown
Member

@k0kubun k0kubun commented Nov 8, 2022 •
edited
Loading

Similarly to Integer#times #8388, this PR rewrites Array#each in Ruby.

microbenchmark

Interpreter

It's a bit slower than the original C method.

$ benchmark-driver benchmark/loop_each.yml -v --chruby 'before;after'
before: ruby 3.4.0dev (2024-01-13T00:28:26Z master f7178045bb) [x86_64-linux]
after: ruby 3.4.0dev (2024-01-13T04:31:50Z builtin-array-each 18c9a45314) [x86_64-linux]
Warming up --------------------------------------
           loop_each      2.129 i/s -       3.000 times in 1.408852s (469.62ms/i)
Calculating -------------------------------------
                         before       after
           loop_each      2.103       1.545 i/s -       6.000 times in 2.852494s 3.882698s

Comparison:
                        loop_each
              before:         2.1 i/s
               after:         1.5 i/s - 1.36x  slower

YJIT

It's 5.3x faster.

$ benchmark-driver benchmark/loop_each.yml -v --chruby 'before::before --yjit-call-threshold=1;after::after --yjit-call-threshold=1'
before: ruby 3.4.0dev (2024-01-13T00:28:26Z master f7178045bb) +YJIT [x86_64-linux]
after: ruby 3.4.0dev (2024-01-13T04:31:50Z builtin-array-each 18c9a45314) +YJIT [x86_64-linux]
Warming up --------------------------------------
           loop_each      2.451 i/s -       3.000 times in 1.223915s (407.97ms/i)
Calculating -------------------------------------
                         before       after
           loop_each      2.456      13.074 i/s -       7.000 times in 2.850521s 0.535404s

Comparison:
                        loop_each
               after:        13.1 i/s
              before:         2.5 i/s - 5.32x  slower

havenwood
havenwood reviewed Nov 10, 2022
View reviewed changes
Comment thread array.rb
@k0kubun k0kubun mentioned this pull request Jan 31, 2023
Merged
@k0kubun k0kubun force-pushed the builtin-array-each branch 11 times, most recently from be9f901 to e291e4f Compare January 31, 2023 23:44
ko1
ko1 reviewed Feb 1, 2023
View reviewed changes
Comment thread enum.c Outdated
nirvdrum
nirvdrum reviewed Feb 1, 2023
View reviewed changes
Comment thread array.rb
@k0kubun k0kubun force-pushed the builtin-array-each branch 2 times, most recently from 2aff775 to 8cfa04c Compare February 2, 2023 00:02
@k0kubun k0kubun mentioned this pull request Feb 2, 2023
Merged
@k0kubun k0kubun force-pushed the builtin-array-each branch from 8cfa04c to 6cd0535 Compare February 2, 2023 00:16
@k0kubun k0kubun mentioned this pull request Feb 2, 2023
Merged
@k0kubun k0kubun force-pushed the builtin-array-each branch 2 times, most recently from ed56476 to bf8d9a3 Compare February 3, 2023 17:41
@k0kubun k0kubun force-pushed the builtin-array-each branch 2 times, most recently from 3f20afb to 224c1f2 Compare February 13, 2023 21:49
@k0kubun k0kubun mentioned this pull request Feb 13, 2023
Closed
eregon
eregon reviewed Mar 16, 2023
View reviewed changes
Comment thread array.rb
@k0kubun k0kubun mentioned this pull request Sep 7, 2023
Merged
@k0kubun k0kubun force-pushed the builtin-array-each branch 3 times, most recently from 4b3a2a4 to 5f8ff7b Compare January 12, 2024 22:48
@k0kubun k0kubun force-pushed the builtin-array-each branch from 5f8ff7b to 18c9a45 Compare January 13, 2024 04:33
@k0kubun k0kubun mentioned this pull request Jan 14, 2024
Merged
@k0kubun
Copy link
Copy Markdown
Member Author

k0kubun commented Jan 18, 2024

Given https://bugs.ruby-lang.org/issues/20182#note-5, #9533 will take this over.

@k0kubun k0kubun closed this Jan 18, 2024
@k0kubun k0kubun deleted the builtin-array-each branch January 18, 2024 21:39
alfuken
alfuken reviewed Feb 2, 2024
View reviewed changes
Comment thread test/ruby/test_gc_compact.rb

Fiber.new {
$ary = OBJ_COUNT.times.map { Foo.new }
$ary.each(&:add_ivars)
Copy link
Copy Markdown

@alfuken alfuken Feb 2, 2024

Choose a reason for hiding this comment

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

@k0kubun forgive me my ignorance, but just out of curiosity: why is there a change in tests from each to reverse_each?.. If this PR is a refactoring of internal mechanism, shouldn't tests remain the same, to ensure (non-)regression integrity?..

Copy link
Copy Markdown
Member Author

@k0kubun k0kubun Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The change of this line was actually not necessary, so I reverted it in the end #9533 (comment).

@tekknolagi tekknolagi mentioned this pull request Feb 6, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ko1 ko1 ko1 left review comments

@eregon eregon eregon left review comments

@tekknolagi tekknolagi tekknolagi left review comments

+3 more reviewers

@nirvdrum nirvdrum nirvdrum left review comments

@alfuken alfuken alfuken left review comments

@havenwood havenwood havenwood 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.

7 participants

@k0kubun @ko1 @nirvdrum @alfuken @eregon @havenwood @tekknolagi