Skip to content

YJIT: Implement codegen for Kernel#block_given? #7202

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 31, 2023
Merged

YJIT: Implement codegen for Kernel#block_given? #7202

merged 1 commit into from
Jan 31, 2023

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Jan 31, 2023

As we plan to convert loop methods to Ruby (e.g. #6687 and #3656), we'll need to introduce an extra Ruby method call on block_given? or work around it with Primitive.block_given_p for such methods.

Since Kernel#block_given? is called almost as frequently as Kernel#respond_to? (see "railsbench" in Shopify/yjit-bench#168), which we already have codegen for, I thought it's probably not a bad idea to just optimize it everywhere.

I also considered using Primitive.attr! 'inline' for this, but the fact that it's CFP-dependent makes it rather complicated to skip pushing a frame. So I feel this is the simplest approach to optimize Kernel#block_given?.

Benchmark

microbenchmark

prelude: |
  def block_given
    block_given?
  end
benchmark:
  - block_given
  - block_given {}
$ benchmark-driver -v ~/tmp/a.yml --chruby 'before::before --yjit;after::after --yjit'
before: ruby 3.3.0dev (2023-01-30T23:43:40Z master 7439ccf0ed) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-01-30T23:59:31Z yjit-block-given-p c93c350a9b) +YJIT [x86_64-linux]
Warming up --------------------------------------
         block_given    37.831M i/s -     38.332M times in 1.013220s (26.43ns/i, 58clocks/i)
      block_given {}    35.519M i/s -     35.813M times in 1.008285s (28.15ns/i, 106clocks/i)
Calculating -------------------------------------
                         before       after
         block_given    44.561M     67.404M i/s -    113.494M times in 2.546934s 1.683793s
      block_given {}    44.189M     56.191M i/s -    106.557M times in 2.411368s 1.896333s

Comparison:
                      block_given
               after:  67403965.4 i/s
              before:  44561158.5 i/s - 1.51x  slower

                   block_given {}
               after:  56191030.5 i/s
              before:  44189397.7 i/s - 1.27x  slower

railsbench

before: ruby 3.3.0dev (2023-01-30T23:43:40Z master 7439ccf0ed) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-01-30T23:59:31Z yjit-block-given-p c93c350a9b) +YJIT [x86_64-linux]

----------  -----------  ----------  ----------  ----------  ------------  -------------
bench       before (ms)  stddev (%)  after (ms)  stddev (%)  before/after  after 1st itr
railsbench  1068.9       0.8         1066.6      1.8         1.00          1.00
----------  -----------  ----------  ----------  ----------  ------------  -------------

@matzbot matzbot requested a review from a team January 31, 2023 00:18
@maximecb maximecb merged commit 2a0bf26 into ruby:master Jan 31, 2023
@maximecb maximecb deleted the yjit-block-given-p branch January 31, 2023 15:11
@XrXr
Copy link
Member

XrXr commented Jan 31, 2023

I think this is a semantic change, no? Now users can override block_given? on their class and change Kernel#loop's behavior.

@k0kubun
Copy link
Member Author

k0kubun commented Jan 31, 2023 •
edited
Loading

We (not me) have occasionally replaced rb_funcall with an equivalent C API as an "optimization". The opposite would be also accepted as long as it doesn't break existing sane or popular implementations. If you break block_given?, many other places could break too, so I don't think that's a valid concern. We don't necessarily use Primitive for every single bulitin Ruby class for that kind of purpose.

@XrXr
Copy link
Member

XrXr commented Jan 31, 2023

Okay that's fair. Sounds like I'm a bit too paranoid about backwards compatibility.

@k0kubun k0kubun mentioned this pull request Feb 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k0kubun @XrXr @maximecb