[pull] master from ruby:master #727
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
~~Two commits:~~
### zjit: Optimize send-with-block to iseq methods
This commit enables JIT-to-JIT calls for send instructions with literal blocks (e.g., `foo { |x| x * 2 }`), rather than falling back to the `rb_vm_send` C wrapper. This optimization applies to both methods with explicit block parameters (`def foo(&block)`) and methods implicitly accepting a block (`def foo; yield if block_given?; end`).
Prior to this change, any callsite with a block would miss out on the JIT-to-JIT fast path and goes through a `rb_vm_send` C wrapper call.
Initially, as Shopify#931 suggested, we assumed this would involve changes to the JIT-to-JIT calling convention to accommodate passing a block argument. However, during implementation, I discovered that @nirvdrum had already wired up the `specval` protocol used by the interpreter in their `invokesuper` work (#887). That infrastructure remained dormant but was exactly what we needed here. After plumbing everything through, it Just Worked™.
It may be possible to design a more direct JIT-to-JIT protocol for passing blocks. In the HIR for `def foo(&block)`, the BB for the JIT entrypoint already takes two arguments (self + &block, presumably), and since `yield` is a keyword, it may be possible to rewrite the implicit case to be explicit (thanks @tenderlove for the idea), and do "better" than passing via `specval`.
I'm not sure if that's a goal eventually, but in any case, if `specval` works, there is no harm in enabling this optimization today.
Implementation notes:
This initial pass largely duplicates the existing `SendWithoutBlock` to `SendWithoutBlockDirect` specialization logic. A future refactor could potentially collapse Send and SendWithoutBlock into a single instruction variant (with `blockiseq: Option<IseqPtr>`, you can always pattern match the Option if needed), since they now follow very similar paths.
However, I wanted to keep this PR focused and also get feedback on that direction first before committing to such a big refactor.
The optimization currently handles `VM_METHOD_TYPE_ISEQ` only. It does not handle "block to block" `VM_METHOD_TYPE_BMETHOD`. It's unclear if that'd be all that difficult, I just didn't try. Happy to do it as a follow-up.
Any callsites not handled by this specialization continue to fallthrough to the existing rb_vm_send harness safely.
Test coverage includes both explicit block parameters and yield-based methods.
Thanks to @tenderlove for initial ideas and walkthrough, and @nirvdrum for the foundation this builds on.
Closes Shopify#931
### ~~zjit: Allow SendWithoutBlockDirect to def f(&blk)~~
Saving this for another time
### Follow-ups
* [ ] Refactor and simplify by merging `hir::Insn::Send` and `hir::Insn::SendWithoutBlock`. (Pending feedback/approval.)
* [ ] Handle block-to-block `VM_METHOD_TYPE_BMETHOD` calls. It appears that `gen_send_iseq_direct` already attempted to handle it.
* [ ] As far as I can tell, we should be able to just enable `super { ... }` too, happy to do that as a follow-up if @nirvdrum doesn't have time for it.
This clamps to supported versions based on the current Ruby version. ruby/prism@eb63748e8b
This improves the `URI.open` method documentation by adding a code example requiring `open-uri` as a basic usage.
When reading the current documentation first, I didn't realize that `open-uri` was required to call the method.
I believe the improved version could be more helpful for new users.
```sh-session
$ ruby -r uri -e 'p URI.open("http://example.com")'
-e:1:in '<main>': private method 'open' called for module URI (NoMethodError)
```
Ref https://docs.ruby-lang.org/en/master/URI.html#method-c-open
Also, this improves formatting with code fonts for better readability.
ruby/open-uri@f4400edc27
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )