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

Merge YJIT: an in-process JIT compiler #4992

Closed
wants to merge 746 commits into from
Closed

Conversation

@maximecb
Copy link
Contributor

@maximecb maximecb commented Oct 19, 2021

This PR introduces YJIT, a just-in-time compiler built using a Lazy Basic Block Versioning (LBBV) compiler architecture. For more details about the technique, please refer to Maxime’s published paper and recorded talks:

YJIT currently provides average speedups of 23% over the CRuby interpreter on realistic benchmarks, and near-instant warm-up time. Plans are to include YJIT in the Ruby 3.1 preview release so that more users can test it out and benefit from potential performance gains.

Current limitations:

  • Disabled by default, must specify --yjit to enable or set RUBY_YJIT_ENABLE=1
  • Currently supports only macOS and Linux.
  • Currently supports only x86-64 CPUs.
  • On unsupported platforms, Ruby runs with the interpreter just like before
  • No garbage collection for generated code. Will produce an error if --yjit-exec-mem-size is exceeded. This should be fixed in the coming months.
  • YJIT uses more memory when active because it needs to allocate machine code. This can be adjusted by setting --yjit-exec-mem-size.

YJIT integrates closely with the interpreter and some core runtime routines such as rb_obj_is_kind_of() and rb_str_eql_internal(). YJIT also introduces a number of callbacks (see yjit.h) so that it can be notified of certain events, such as constants or methods being redefined. These changes are necessary so that YJIT can deoptimize code. These callbacks could potentially be useful for MJIT as well.

YJIT does not spawn an external compiler as a sub-process for compilation. YJIT compiles concurrently with Ruby code execution on the same thread. Despite this, it provides near-instant warm-up. That is, YJIT typically delivers a speedup on the first iteration of our benchmarks.

Shopify is committed to making sure that YJIT remains in good working order. This PR adds new test workflows to help detect issues early on. Should bugs and issues arise, we will assist in investigating and fixing these in a timely manner. If necessary, code generation for specific YARV instructions can be disabled by commenting a single line near the end of yjit_codegen.c.

We have tried to keep the YJIT code clean and approachable. The codebase is well-commented. We have a number of non-Shopify contributors who have found YJIT and submitted code.

YJIT makes additional runtime checks if compiled with RUBY_DEBUG > 0. It can also collect runtime statistics if compiled with RUBY_DEBUG or YJIT_STATS > 0 and also run with the --yjit-stats option or the RUBY_YJIT_STATS environment variable set to 1.

YJIT cannot be enabled at the same time as MJIT. Both can be compiled into Ruby, but only one can be active at runtime.

Contributors to the YJIT project so far:
Alan Wu
Aaron Patterson
Benson Muite
Dylan Smith
Eileen Uchitelle
Max Bernstein
Marc Feeley
Jose Narvaez
Jean Boussier
John Hawthorn
Kevin Newton
Maxime Chevalier-Boisvert
Mike Dalessio
Noah Gibbs
Takashi Kokubun
Ufuk Kayserilioglu

@maximecb
Copy link
Contributor Author

@maximecb maximecb commented Oct 19, 2021

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 19, 2021

must specify --yjit to enable or set YJIT_RUBY_ENABLE=1
run with the --yjit-stats option or the RUBY_YJIT_STATS environment variable set to 1.

Is there any motivation to support YJIT_RUBY_ENABLE/RUBY_YJIT_STATS environment variables while you can also use RUBYOPT="--yjit --yjit-status"?

@noahgibbs
Copy link
Contributor

@noahgibbs noahgibbs commented Oct 19, 2021

Is there any motivation to support YJIT_RUBY_ENABLE/RUBY_YJIT_STATS environment variables
while you can also use RUBYOPT="--yjit --yjit-status"?

Primarily so we can activate YJIT or stats without having to worry about preserving/changing RUBYOPT, which may contain other options.

@maximecb
Copy link
Contributor Author

@maximecb maximecb commented Oct 19, 2021

It's more convenient for some of the build systems we use in production, where it can be tricky to change command-line options for some ruby builds and not others.

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 20, 2021

got it.

Another point: I see the following when I try to switch it to "Rebase and merge":

Screen Shot 2021-10-19 at 5 29 07 PM

Given that ruby/ruby disables merge commits, the only remaining option is "Squash and merge". Would that be okay for you, or would you like to take a look at making it possible to use "Rebase and merge" to keep commit history?

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 20, 2021

I tried the rebase locally, and it succeeded. It's probably just a limitation of GitHub. We can manually rebase and push it.

yjit_init_codegen();

// YJIT Ruby module
mYjit = rb_define_module("YJIT");
Copy link
Member

@k0kubun k0kubun Oct 20, 2021

Choose a reason for hiding this comment

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

Can we put this under RubyVM::? People generally prefer putting CRuby-specific components under RubyVM. I think we don't expect it to be ported to JRuby/TruffleRuby at least.

Copy link
Contributor Author

@maximecb maximecb Oct 20, 2021

Choose a reason for hiding this comment

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

We thought it was convenient to have a YJIT module for YJIT-specific things. Typical use case:

YJIT.reset_stats! if defined?(YJIT.reset_stats!)

Do you mean we should move this into RubyVM::YJIT ? If this is the preferred way, we will make that change as requested after the merge.

Copy link
Member

@byroot byroot Oct 20, 2021

Choose a reason for hiding this comment

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

Do you mean we should move this into RubyVM::YJIT ?

Yes.

I think that change makes sense.

Copy link
Member

@k0kubun k0kubun Oct 20, 2021

Choose a reason for hiding this comment

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

Given that RubyVM::MJIT was approved "because RubyVM is just for internal features" https://bugs.ruby-lang.org/issues/14830#note-3, I just thought it'd be easier to get the approval of RubyVM::YJIT than top-level YJIT, but you can try proposing it without RubyVM:: this time.

I think we can skip the discussion for the preview release, however, before the actual 3.1 release, we may need another ticket on https://bugs.ruby-lang.org to discuss user-facing Ruby interface changes. We usually discuss such changes (e.g. YJIT and its methods, the command-line options, and the environment variables) on https://bugs.ruby-lang.org/ before merging them.

P.S. You may want to put something at https://rubygems.org/gems/yjit to reserve the namespace. (e.g. https://rubygems.org/gems/ractor https://rubygems.org/gems/mjit)

Copy link
Contributor Author

@maximecb maximecb Oct 20, 2021

Choose a reason for hiding this comment

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

I think we're totally ok with moving the module into RubyVM::YJIT to match existing conventions. It won't be a problem. We should be able to open a PR for that in the coming week.

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 20, 2021

I think it's good enough for the first merge. We can address minor issues after that, including my above comment.

Now that Matz gave the approval to make you a committer, how would you like to finish merging it? We can either wait for you to go through https://bugs.ruby-lang.org/projects/ruby/wiki/CommitterHowto#What-to-do-for-registering-you-as-a-committer and let you merge it yourself, or let me push them to unblock the preview release early while doing the process in case it takes a long time.

@maximecb
Copy link
Contributor Author

@maximecb maximecb commented Oct 20, 2021

Hello @k0kubun. Thank you for reviewing! Happy to see that we are passing the CI tests.

We just found that there is a bug in our keyword argument handling in YJIT that shows up in production. We are hoping to fix this today and update this PR, add another test. If we can't fix it today we will just disable the feature and still be able to merge today.

I am ok with you merging the PR 👍 Is it ok if we take a few hours to look into the keyword argument problem today?

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 20, 2021

We are hoping to fix this today and update this PR, add another test. If we can't fix it today we will just disable the feature and still be able to merge today.

I am ok with you merging the PR 👍 Is it ok if we take a few hours to look into the keyword argument problem today?

Sounds good 👍 Thank you.

edit: For the record, Alan contacted me about taking over the merge and I happily handed it over to him 🙂

noahgibbs and others added 18 commits Oct 20, 2021
In jit_guard_known_klass whenever we encounter a new class should
recompile the current instruction.

However, previously once jit_guard_known_klass had guarded for a heap
object it would not recompile for  any immediate (special const) objects
arriving afterwards and would take a plain side-exit instead of a chain
guard.

This commit uses jit_chain_guard inside jit_guard_known_klass instead of
the plain side exit, so that we can recompile for any special constants
arriving afterwards.
Basically the same thing as opt_mod, but for multiplying.
This script is an lldb helper that just loops through all the comments
stored and prints out the comment along with the address corresponding
to the comment.

For example, I'm crashing in JIT code at address 0x0000000110000168.
Using the `lc` helper I can see that it's probably crashing inside the
exit back to the interpreter

```
(lldb) bt 5
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x22220021)
    frame #0: 0x0000000110000168
  * frame #1: 0x00000001002b5ff5 miniruby`invoke_block_from_c_bh [inlined] invoke_block(ec=0x0000000100e05350, iseq=0x0000000100c1ff10, self=0x0000000100c76cc0, captured=<unavailable>, cref=0x0000000000000000, type=<unavailable>, opt_pc=<unavailable>) at vm.c:1268:12
    frame #2: 0x00000001002b5f7d miniruby`invoke_block_from_c_bh [inlined] invoke_iseq_block_from_c(ec=<unavailable>, captured=<unavailable>, self=0x0000000100c76cc0, argc=2, argv=<unavailable>, kw_splat=0, passed_block_handler=0x0000000000000000, cref=0x0000000000000000, is_lambda=<unavailable>, me=0x0000000000000000) at vm.c:1340
    frame #3: 0x00000001002b5e14 miniruby`invoke_block_from_c_bh(ec=<unavailable>, block_handler=<unavailable>, argc=<unavailable>, argv=<unavailable>, kw_splat=0, passed_block_handler=0x0000000000000000, cref=0x0000000000000000, is_lambda=<unavailable>, force_blockarg=0) at vm.c:1358
    frame #4: 0x000000010029860b miniruby`rb_yield_values(n=<unavailable>) at vm_eval.c:0
(lldb) lc
0x11000006d "putobject_INT2FIX_1_"
0x110000083 "leave"
0x110000087 "check for interrupts"
0x110000087 "RUBY_VM_CHECK_INTS(ec)"
0x110000098 "check for finish frame"
0x1100000ed "getlocal_WC_0"
0x110000107 "getlocal_WC_1"
0x11000012a "opt_send_without_block"
0x110000139 "opt_send_without_block"
0x11000013c "exit to interpreter"
```
The last parameter to rb_struct_define_under needs to be NULL otherwise
we can get a SEGV.
We clear locals when we know their values might change (ex. when
performing a method call). However previously values on the stack which
were originally pushed from a local would still point back to that
local.

With this commit, when clearing locals, we'll now iterate over the
mappings of the stack and copy the known type from the local to the
stack mapping, removing the association to the local.

This should mean both that we'll retain any information we already know
about the local type, and that if a local is modified we won't
incorrectly infer it's new type from the existing value on the stack.
XrXr and others added 19 commits Oct 20, 2021
Since opt_getinlinecache and opt_setinlinecache point to the same cache
struct, there is no need to track the index of the get instruction and
then store it on the cache struct later when processing the set
instruction. Setting it when processing the get instruction works just
as well.

This change reduces our diff.
This reverts commit 60f3f25.
We don't need to pass --disable-yjit when running MJIT tests anymore
because we are off by default.
Previously, options such as "--yjit123" would enable YJIT. Additionally,
the error message for argument parsing mentioned "--jit-..." instead of
"--yjit-...".
When YJIT make calls to routines without reconstructing interpreter
state through jit_prepare_routine_call(), it relies on the routine to
never allocate, raise, and push/pop control frames. Comment about this
on the routines that YJTI calls.

This is probably something we should dynamically verify on debug builds.
It's hard to statically verify this as it requires verifying all
functions in the call tree. Maybe something to look at in the future.
Since conventionally scripts don't live at the top level of the repo.
In an effort to simplify the logic YJIT generates for accessing instance
variable, YJIT ensures that a given name-to-index mapping exists at
compile time. In the case that the mapping doesn't exist, it was created
by using rb_ivar_set() with Qundef on the sample object we see at
compile time. This hack isn't fine if the sample object happens to be
frozen, in which case YJIT would raise a FrozenError unexpectedly.

To deal with this, make a new function that only reserves the mapping
but doesn't touch the object. This is rb_obj_ensure_iv_index_mapping().
This new function superceeds the functionality of rb_iv_index_tbl_lookup()
so it was removed.

Reported by and includes a test case from John Hawthorn <john@hawthorn.email>

Fixes: rubyGH-282
So we don't try to run x64 on ARM.
Previously, we were shuffling keyword arguments before checking for
interrupts. In the case that we side exit in the interrupt check,
we left the interpreter with an already-shuffled argument list for
the call, resulting in a double shuffle, leaving the locals in the
wrong order for the callee.

Do keyword shuffling after all the possible side exits.

Co-authored-by: Kevin Newton <kddnewton@gmail.com>
@XrXr XrXr force-pushed the yjit_upstream_pr branch from 519ce78 to abf0532 Oct 20, 2021
XrXr added 2 commits Oct 20, 2021
On non RUBY_DEBUG builds, assert() compiles to nothing and the compiler
warns about uninitialized variables in those code paths. Replace
those asserts with rb_bug() to fix the warnings and do the assert in
all builds. Since yjit_asm_tests.c compiles outside of Ruby, it needed
a distinct version of rb_bug().

Also put YJIT_STATS check for function delcaration that is only defined
in YJIT_STATS builds.
Fixes:
    ./src/yjit_asm.c:196:8: warning: 'mem_block' may be used uninitialized [-Wmaybe-uninitialized]
@XrXr
Copy link
Member

@XrXr XrXr commented Oct 20, 2021

I merged it: 6a9e2b3

@XrXr XrXr closed this Oct 20, 2021
@hsbt
Copy link
Member

@hsbt hsbt commented Oct 21, 2021

@maximecb @XrXr Thanks for your works. But this large pull-request broke our dev environment like slack-bot, email notification and more. Please consider to squash or minimum step of merge process in the next work.

@maximecb
Copy link
Contributor Author

@maximecb maximecb commented Oct 21, 2021

Understood. Sorry about the many commits 😅

@kapso
Copy link

@kapso kapso commented Dec 31, 2021

BTW I tried YJIT_RUBY_ENABLE and it does not work, but RUBY_YJIT_ENABLE does work. I tested this using RubyVM::YJIT.runtime_stats

Also RUBYOPT=--yjit and RUBYOPT=--enable-yjit work as well.

Picked this from here - https://shopify.engineering/yjit-faster-rubying

@maximecb
Copy link
Contributor Author

@maximecb maximecb commented Jan 4, 2022

Hi @noahgibbs ! Would it be possible to update the blog post?

@noahgibbs
Copy link
Contributor

@noahgibbs noahgibbs commented Jan 4, 2022

Yup! I'll get on that.

@noahgibbs
Copy link
Contributor

@noahgibbs noahgibbs commented Jan 5, 2022

Hm. RUBY_YJIT_ENABLE is what I'm seeing in both the blog post and in the Ruby source code. I think we may have had it the other way some time back but changed it already? But what I'm seeing there now looks right, and I'm not finding YJIT_RUBY_ENABLE currently. @kapso, can you shift-reload the blog post in your browser and make sure you're seeing the latest version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet