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

YJIT: Support nil and blockparamproxy as blockarg in send #6492

Merged
merged 1 commit into from Oct 26, 2022

Conversation

matthewd
Copy link
Contributor

@matthewd matthewd commented Oct 5, 2022

Here, @jhawthorn and I have implemented support for the simple but common cases of supplying a block to send via a blockparamproxy (or a nil). Because the blockparamproxy only appears in controlled circumstances, we are able to rely entirely on the type system to recognise this case, without runtime checks.

To support that, we've added a new Type::BlockParamProxy. It mostly fits in there with the other singleton types like Type::True, though it's unique in that it's not accessible from Ruby code (despite being a real VALUE object).

We've also moved some logic into gen_push_frame, turning CurrentFrame into a parameterized BlockISeq(IseqPtr). In the process, we've folded in PrevEP, renaming the enum from BlockHandler to SpecVal. They're mutually exclusive (they both end up storing their value in the same place), so this allows the type system to enforce that.

@matzbot matzbot requested a review from a team October 5, 2022 05:53
@jhawthorn
Copy link
Member

jhawthorn commented Oct 5, 2022

This reduces total exits by 9%, from 2509837 to 2275480

Before:

method call exit reasons: 
                block_arg     299619 (40.8%)
    optimized_method_call     177089 (24.1%)
...

After:

method call exit reasons: 
    optimized_method_call     177089 (33.4%)
      iseq_complex_callee     143880 (27.1%)
              iseq_zsuper      46021 ( 8.7%)
    optimized_method_send      39761 ( 7.5%)
      args_splat_non_iseq      36747 ( 6.9%)
                block_arg      33595 ( 6.3%)

bootstraptest/test_yjit.rb Outdated Show resolved Hide resolved
yjit/src/codegen.rs Outdated Show resolved Hide resolved
Comment on lines +4039 to 4022
enum SpecVal {
None,
CurrentFrame,
BlockISeq(IseqPtr),
BlockParamProxy,
PrevEP(*const VALUE),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also like a comment explaining what SpecVal is generally used for. In general, it's best not to assume that people are familiar with all the details of the CRuby codebase and send imo.

yjit/src/core.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

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

Looks good over all with some minor comments and one test failure.

@maximecb
Copy link
Contributor

maximecb commented Oct 5, 2022

This reduces total exits by 9%, from 2509837 to 2275480

Before:

method call exit reasons: 
                block_arg     299619 (40.8%)
    optimized_method_call     177089 (24.1%)
...

After:

method call exit reasons: 
    optimized_method_call     177089 (33.4%)
      iseq_complex_callee     143880 (27.1%)
              iseq_zsuper      46021 ( 8.7%)
    optimized_method_send      39761 ( 7.5%)
      args_splat_non_iseq      36747 ( 6.9%)
                block_arg      33595 ( 6.3%)

Nice 👌

Copy link
Member

@XrXr XrXr left a comment

Choose a reason for hiding this comment

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

Looks nice!

yjit/src/codegen.rs Outdated Show resolved Hide resolved
yjit/src/codegen.rs Outdated Show resolved Hide resolved
yjit/src/core.rs Outdated Show resolved Hide resolved
@matthewd matthewd force-pushed the send-blockarg branch 2 times, most recently from 90717dd to d4c1004 Compare October 26, 2022 01:45
@k0kubun
Copy link
Member

k0kubun commented Oct 26, 2022

FYI for the Arm CI failure:

$ cat /tmp/a.rb
define_method(:foo) do |&block|
  block.call
end

def bar(&block)
  foo(&block)
end

bar { 1 }
bar { 2 }

$ RUST_BACKTRACE=1 ruby --disable-gems --yjit-verify-ctx --yjit-call-threshold=1 /tmp/a.rb
thread '<unnamed>' panicked at 'verify_ctx: ctx type (BlockParamProxy) incompatible with actual value on stack: T_OBJECT', src/codegen.rs:352:13
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: yjit::codegen::verify_ctx
             at ./yjit/src/codegen.rs:352:13
   3: yjit::codegen::gen_single_block
             at ./yjit/src/codegen.rs:778:13
   4: yjit::core::gen_block_series_body
             at ./yjit/src/core.rs:1440:23
   5: yjit::core::gen_block_series
             at ./yjit/src/core.rs:1418:18
   6: yjit::core::branch_stub_hit_body
             at ./yjit/src/core.rs:1774:17
   7: yjit::core::branch_stub_hit::{{closure}}
             at ./yjit/src/core.rs:1678:13
   8: std::panicking::try::do_call
             at /private/tmp/rust-20220719-52938-vitvrw/rustc-1.62.1-src/library/std/src/panicking.rs:492:40
   9: ___rust_try
  10: std::panicking::try
             at /private/tmp/rust-20220719-52938-vitvrw/rustc-1.62.1-src/library/std/src/panicking.rs:456:19
  11: std::panic::catch_unwind
             at /private/tmp/rust-20220719-52938-vitvrw/rustc-1.62.1-src/library/std/src/panic.rs:137:14
  12: yjit::cruby::with_vm_lock
             at ./yjit/src/cruby.rs:577:21
  13: yjit::core::branch_stub_hit
             at ./yjit/src/core.rs:1677:9
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
YJIT panicked while holding VM lock acquired at src/core.rs:1677. Aborting...
zsh: abort      RUST_BACKTRACE=1 ruby --disable-gems --yjit-verify-ctx --yjit-call-threshold=

Co-authored-by: John Hawthorn <john@hawthorn.email>
@k0kubun
Copy link
Member

k0kubun commented Oct 26, 2022

Copy link
Contributor

@maximecb maximecb left a comment

Choose a reason for hiding this comment

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

Good work! :)

@maximecb maximecb merged commit c746f38 into ruby:master Oct 26, 2022
tenderlove pushed a commit to Shopify/ruby that referenced this pull request Oct 27, 2022
Co-authored-by: John Hawthorn <john@hawthorn.email>

Co-authored-by: John Hawthorn <john@hawthorn.email>
@matthewd matthewd deleted the send-blockarg branch October 28, 2022 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants