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.

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

YJIT: Add RubyVM::YJIT.enable #8705

Merged
merged 1 commit into from Oct 19, 2023
Merged

YJIT: Add RubyVM::YJIT.enable #8705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 8 additions & 21 deletions cont.c
View file
Open in desktop
Expand Up @@ -71,8 +71,6 @@ static VALUE rb_cFiberPool;
#define FIBER_POOL_ALLOCATION_FREE
#endif

#define jit_cont_enabled (rb_rjit_enabled || rb_yjit_enabled_p())

enum context_type {
CONTINUATION_CONTEXT = 0,
FIBER_CONTEXT = 1
Expand Down Expand Up @@ -1062,10 +1060,8 @@ cont_free(void *ptr)

RUBY_FREE_UNLESS_NULL(cont->saved_vm_stack.ptr);

if (jit_cont_enabled) {
VM_ASSERT(cont->jit_cont != NULL);
jit_cont_free(cont->jit_cont);
}
VM_ASSERT(cont->jit_cont != NULL);
jit_cont_free(cont->jit_cont);
/* free rb_cont_t or rb_fiber_t */
ruby_xfree(ptr);
RUBY_FREE_LEAVE("cont");
Expand Down Expand Up @@ -1311,9 +1307,6 @@ rb_jit_cont_each_iseq(rb_iseq_callback callback, void *data)
void
rb_jit_cont_finish(void)
{
if (!jit_cont_enabled)
return;

struct rb_jit_cont *cont, *next;
for (cont = first_jit_cont; cont != NULL; cont = next) {
next = cont->next;
Expand All @@ -1326,9 +1319,8 @@ static void
cont_init_jit_cont(rb_context_t *cont)
{
VM_ASSERT(cont->jit_cont == NULL);
if (jit_cont_enabled) {
cont->jit_cont = jit_cont_new(&(cont->saved_ec));
}
// We always allocate this since YJIT may be enabled later
cont->jit_cont = jit_cont_new(&(cont->saved_ec));
}

struct rb_execution_context_struct *
Expand Down Expand Up @@ -1375,15 +1367,11 @@ rb_fiberptr_blocking(struct rb_fiber_struct *fiber)
return fiber->blocking;
}

// Start working with jit_cont.
// Initialize the jit_cont_lock
void
rb_jit_cont_init(void)
{
if (!jit_cont_enabled)
return;

rb_native_mutex_initialize(&jit_cont_lock);
cont_init_jit_cont(&GET_EC()->fiber_ptr->cont);
}

#if 0
Expand Down Expand Up @@ -2564,10 +2552,9 @@ rb_threadptr_root_fiber_setup(rb_thread_t *th)
fiber->killed = 0;
fiber_status_set(fiber, FIBER_RESUMED); /* skip CREATED */
th->ec = &fiber->cont.saved_ec;
// When rb_threadptr_root_fiber_setup is called for the first time, rb_rjit_enabled and
// rb_yjit_enabled_p() are still false. So this does nothing and rb_jit_cont_init() that is
// called later will take care of it. However, you still have to call cont_init_jit_cont()
// here for other Ractors, which are not initialized by rb_jit_cont_init().
// This is the first fiber. Hence it's the first jit_cont_new() as well.
// Initialize the mutex for jit_cont_new() in cont_init_jit_cont().
rb_jit_cont_init();
cont_init_jit_cont(&fiber->cont);
}

Expand Down
4 changes: 0 additions & 4 deletions ruby.c
View file
Open in desktop
Expand Up @@ -1796,10 +1796,6 @@ ruby_opt_init(ruby_cmdline_options_t *opt)
if (opt->yjit)
rb_yjit_init();
#endif
// rb_threadptr_root_fiber_setup for the initial thread is called before rb_yjit_enabled_p()
// or rjit_enabled becomes true, meaning jit_cont_new is skipped for the initial root fiber.
// Therefore we need to call this again here to set the initial root fiber's jit_cont.
rb_jit_cont_init(); // must be after rjit_enabled = true and rb_yjit_init()

ruby_set_script_name(opt->script_name);
require_libraries(&opt->req_list);
Expand Down
37 changes: 23 additions & 14 deletions test/ruby/test_yjit.rb
View file
Open in desktop
Expand Up @@ -51,27 +51,36 @@ def test_command_line_switches
#assert_in_out_err('--yjit-call-threshold=', '', [], /--yjit-call-threshold needs an argument/)
end

def test_starting_paused
program = <<~RUBY
def test_yjit_enable
args = []
args << "--disable=yjit" if RubyVM::YJIT.enabled?
assert_separately(args, <<~RUBY)
assert_false RubyVM::YJIT.enabled?
assert_false RUBY_DESCRIPTION.include?("+YJIT")

RubyVM::YJIT.enable

assert_true RubyVM::YJIT.enabled?
assert_true RUBY_DESCRIPTION.include?("+YJIT")
RUBY
end

def test_yjit_enable_with_call_threshold
assert_separately(%w[--yjit-disable --yjit-call-threshold=1], <<~RUBY)
def not_compiled = nil
def will_compile = nil
def compiled_counts = RubyVM::YJIT.runtime_stats[:compiled_iseq_count]
counts = []
def compiled_counts = RubyVM::YJIT.runtime_stats&.dig(:compiled_iseq_count)

not_compiled
counts << compiled_counts
assert_nil compiled_counts
assert_false RubyVM::YJIT.enabled?

RubyVM::YJIT.resume
RubyVM::YJIT.enable

will_compile
counts << compiled_counts

if counts[0] == 0 && counts[1] > 0
p :ok
end
assert compiled_counts > 0
assert_true RubyVM::YJIT.enabled?
RUBY
assert_in_out_err(%w[--yjit-pause --yjit-stats --yjit-call-threshold=1], program, success: true) do |stdout, stderr|
assert_equal([":ok"], stdout)
end
end

def test_yjit_stats_and_v_no_error
Expand Down
26 changes: 19 additions & 7 deletions version.c
View file
Open in desktop
Expand Up @@ -141,20 +141,15 @@ Init_version(void)

int ruby_mn_threads_enabled;

void
Init_ruby_description(ruby_cmdline_options_t *opt)
static void
define_ruby_description(const char *const jit_opt)
{
static char desc[
sizeof(ruby_description)
+ rb_strlen_lit(YJIT_DESCRIPTION)
+ rb_strlen_lit(" +MN")
];

const char *const jit_opt =
RJIT_OPTS_ON ? " +RJIT" :
YJIT_OPTS_ON ? YJIT_DESCRIPTION :
"";

const char *const threads_opt = ruby_mn_threads_enabled ? " +MN" : "";

int n = snprintf(desc, sizeof(desc),
Expand All @@ -176,6 +171,23 @@ Init_ruby_description(ruby_cmdline_options_t *opt)
rb_define_global_const("RUBY_DESCRIPTION", /* MKSTR(description) */ description);
}

void
Init_ruby_description(ruby_cmdline_options_t *opt)
{
const char *const jit_opt =
RJIT_OPTS_ON ? " +RJIT" :
YJIT_OPTS_ON ? YJIT_DESCRIPTION :
"";
define_ruby_description(jit_opt);
}

void
ruby_set_yjit_description(void)
{
rb_const_remove(rb_cObject, rb_intern("RUBY_DESCRIPTION"));
define_ruby_description(YJIT_DESCRIPTION);
}

Comment on lines +174 to +190
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR looks good and simplifies a lot of things 👍

This is the only part I'm not sure about. It's not fully clear what the Ruby description is going to print. Does it only print +YJIT if YJIT is enabled, which can happen later during execution?

I feel like ideally, the description should tell us that this is a YJIT-capable build of CRuby, so maybe it should say something like +YJIT (enabled) or +YJIT (disabled) ? Otherwise, as a simpler solution, we could just always have +YJIT, and people would have to query RubyVM::YJIT::enabled? if they want to know if YJIT is enabled or not.

Copy link
Member Author

@k0kubun k0kubun Oct 19, 2023 •
edited

Choose a reason for hiding this comment

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

Does it only print +YJIT if YJIT is enabled, which can happen later during execution?

Correct.

This PR doesn't change the format. If disabled, that part is "". If enabled, that part is "+YJIT", "+YJIT dev", "+YJIT dev_nodebug", or "+YJIT stats". The whole point of this PR's design is to avoid confusion, so I wouldn't introduce "+YJIT (disabled)" (also that state is in fact the same as just running the interpreter).

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing that may be confusing is that the "+YJIT" is going to appear only when YJIT is enabled, which is going to happen much later if YJIT is enabled in software. You won't see it if you run ruby -v.

That's why I think YJIT enabled and YJIT disabled make sense to report. It lets us know we have a YJIT-capable build of CRuby.

Copy link
Member Author

@k0kubun k0kubun Oct 19, 2023 •
edited

Choose a reason for hiding this comment

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

I should have asked this first: what problem do you want to solve with this? Does it really require RUBY_DESCRIPTION to have +YJIT? If you're concerned about possibly breaking RubyVM::YJIT.enable when Ruby is not built with --enable-yjit and troubleshooting it is hard, for example, can we just tell that it's not built with --enable-yjit when RubyVM::YJIT.enable is called? If you want to quickly check if it's built with --enable-yjit, why not just run ruby --yjit -v?

In CRuby, we generally present build configurations in RbConfig::CONFIG instead of RUBY_DESCRIPTION. RbConfig::CONFIG["YJIT_SUPPORT"] is "yes" or "no". Can we use that information in RubyVM::YJIT APIs or warning/error messages to deal with the problem? Would you like to introduce RubyVM::YJIT.supported? to wrap it?

The thing that may be confusing

To me, what seems more confusing to end users is "+YJIT (disabled)". "+YJIT" seems to say YJIT is enabled, but "(disabled)" also says it's disabled. "+YJIT (disabled)" seems just as confusing as "YJIT is enabled but paused".

I think the "+XXX" format is generally used when it's actually enabled, not when it's capable of doing it. For example, JRuby prints +indy and +jit when the flags are passed, and doesn't print +indy (disabled) or +jit (disabled) when built with that support but disabled.

Similarly, today's CRuby prints +MN when RUBY_MN_THREADS=1 is given, and doesn't print +MN (disabled) just by the fact that USE_MN_THREADS macro is 1 when it's disabled. It seems inconsistent in CRuby alone too.

One last note, changing the format of RUBY_DESCRIPTION for the YJIT-disabled mode might need discussions on bugs.ruby-lang.org and Matz's approval unlike RubyVM::YJIT-closed changes like the current diff. I would rather merge this first without breaking compatibility to make sure we land this feature in Ruby 3.3, and then spend time discussing the format of RUBY_DESCRIPTION, which is less important than adding RubyVM::YJIT.enable in 3.3.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to know if we're running a YJIT-capable version of Ruby when doing ruby -v, that would be the use case, but I get that you don't want to break convention with other flags shown in the Ruby description.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've always wanted a quick way to check if --enable-shared because it does slow down the interpreter and macOS and ruby-build unfortunately turn it on by default. I wish ruby -v printed it, so I guess I get your use case.

I would still want something less confusing than +YJIT (disable), though. If there's no concise way to communicate it in ruby -v, maybe we could make ruby -vv more verbose and let it print important build configurations like YJIT_SUPPORT and ENABLE_SHARED. Either way, we'd need a ticket to introduce such changes. Until then, ruby -v --yjit seems like a good enough compromise.

void
ruby_show_version(void)
{
Expand Down
7 changes: 3 additions & 4 deletions vm.c
View file
Open in desktop
Expand Up @@ -426,15 +426,14 @@ jit_compile(rb_execution_context_t *ec)
{
const rb_iseq_t *iseq = ec->cfp->iseq;
struct rb_iseq_constant_body *body = ISEQ_BODY(iseq);
bool yjit_enabled = rb_yjit_compile_new_iseqs();
if (!(yjit_enabled || rb_rjit_call_p)) {
if (!(rb_yjit_enabled_p || rb_rjit_call_p)) {
return NULL;
}

// Increment the ISEQ's call counter and trigger JIT compilation if not compiled
if (body->jit_entry == NULL) {
body->jit_entry_calls++;
if (yjit_enabled) {
if (rb_yjit_enabled_p) {
if (rb_yjit_threshold_hit(iseq, body->jit_entry_calls)) {
rb_yjit_compile_iseq(iseq, ec, false);
}
Expand Down Expand Up @@ -476,7 +475,7 @@ jit_compile_exception(rb_execution_context_t *ec)
{
const rb_iseq_t *iseq = ec->cfp->iseq;
struct rb_iseq_constant_body *body = ISEQ_BODY(iseq);
if (!rb_yjit_compile_new_iseqs()) {
if (!rb_yjit_enabled_p) {
return NULL;
}

Expand Down
2 changes: 1 addition & 1 deletion vm_method.c
View file
Open in desktop
Expand Up @@ -200,7 +200,7 @@ clear_method_cache_by_id_in_class(VALUE klass, ID mid)
struct rb_id_table *cm_tbl;
if ((cm_tbl = RCLASS_CALLABLE_M_TBL(klass)) != NULL) {
VALUE cme;
if (rb_yjit_enabled_p() && rb_id_table_lookup(cm_tbl, mid, &cme)) {
if (rb_yjit_enabled_p && rb_id_table_lookup(cm_tbl, mid, &cme)) {
rb_yjit_cme_invalidate((rb_callable_method_entry_t *)cme);
}
if (rb_rjit_enabled && rb_id_table_lookup(cm_tbl, mid, &cme)) {
Expand Down
11 changes: 3 additions & 8 deletions yjit.c
View file
Open in desktop
Expand Up @@ -1171,20 +1171,15 @@ VALUE rb_yjit_insns_compiled(rb_execution_context_t *ec, VALUE self, VALUE iseq)
VALUE rb_yjit_code_gc(rb_execution_context_t *ec, VALUE self);
VALUE rb_yjit_simulate_oom_bang(rb_execution_context_t *ec, VALUE self);
VALUE rb_yjit_get_exit_locations(rb_execution_context_t *ec, VALUE self);
VALUE rb_yjit_resume(rb_execution_context_t *ec, VALUE self);
VALUE rb_yjit_enable(rb_execution_context_t *ec, VALUE self);

// Preprocessed yjit.rb generated during build
#include "yjit.rbinc"

// Can raise RuntimeError
// Initialize the GC hooks
void
rb_yjit_init(void)
rb_yjit_init_gc_hooks(void)
{
// Call the Rust initialization code
void rb_yjit_init_rust(void);
rb_yjit_init_rust();

// Initialize the GC hooks. Do this second as some code depend on Rust initialization.
struct yjit_root_struct *root;
VALUE yjit_root = TypedData_Make_Struct(0, struct yjit_root_struct, &yjit_root_type, root);
rb_gc_register_mark_object(yjit_root);
Expand Down
6 changes: 2 additions & 4 deletions yjit.h
View file
Open in desktop
Expand Up @@ -28,9 +28,8 @@
extern uint64_t rb_yjit_call_threshold;
extern uint64_t rb_yjit_cold_threshold;
extern uint64_t rb_yjit_live_iseq_count;
extern bool rb_yjit_enabled_p;
void rb_yjit_incr_counter(const char *counter_name);
bool rb_yjit_enabled_p(void);
bool rb_yjit_compile_new_iseqs(void);
void rb_yjit_invalidate_all_method_lookup_assumptions(void);
void rb_yjit_cme_invalidate(rb_callable_method_entry_t *cme);
void rb_yjit_collect_binding_alloc(void);
Expand All @@ -51,9 +50,8 @@ void rb_yjit_show_usage(int help, int highlight, unsigned int width, int columns
// !USE_YJIT
// In these builds, YJIT could never be turned on. Provide dummy implementations.

#define rb_yjit_enabled_p false
static inline void rb_yjit_incr_counter(const char *counter_name) {}
static inline bool rb_yjit_enabled_p(void) { return false; }
static inline bool rb_yjit_compile_new_iseqs(void) { return false; }
static inline void rb_yjit_invalidate_all_method_lookup_assumptions(void) {}
static inline void rb_yjit_cme_invalidate(rb_callable_method_entry_t *cme) {}
static inline void rb_yjit_collect_binding_alloc(void) {}
Expand Down
8 changes: 4 additions & 4 deletions yjit.rb
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@
module RubyVM::YJIT
# Check if YJIT is enabled
def self.enabled?
Primitive.cexpr! 'RBOOL(rb_yjit_enabled_p())'
Primitive.cexpr! 'RBOOL(rb_yjit_enabled_p)'
end

# Check if --yjit-stats is used.
Expand All @@ -29,9 +29,9 @@ def self.reset_stats!
Primitive.rb_yjit_reset_stats_bang
end

# Resume YJIT compilation after paused on startup with --yjit-pause
def self.resume
Primitive.rb_yjit_resume
# Enable YJIT compilation.
def self.enable
Primitive.rb_yjit_enable
end

# If --yjit-trace-exits is enabled parse the hashes from
Expand Down
12 changes: 6 additions & 6 deletions yjit/src/options.rs
View file
Open in desktop
Expand Up @@ -47,9 +47,9 @@ pub struct Options {
// how often to sample exit trace data
pub trace_exits_sample_rate: usize,

// Whether to start YJIT in paused state (initialize YJIT but don't
// compile anything)
pub pause: bool,
// Whether to enable YJIT at boot. This option prevents other
// YJIT tuning options from enabling YJIT at boot.
pub disable: bool,

/// Dump compiled and executed instructions for debugging
pub dump_insns: bool,
Expand Down Expand Up @@ -81,7 +81,7 @@ pub static mut OPTIONS: Options = Options {
gen_trace_exits: false,
print_stats: true,
trace_exits_sample_rate: 0,
pause: false,
disable: false,
dump_insns: false,
dump_disasm: None,
verify_ctx: false,
Expand Down Expand Up @@ -186,8 +186,8 @@ pub fn parse_option(str_ptr: *const std::os::raw::c_char) -> Option<()> {
}
},

("pause", "") => unsafe {
OPTIONS.pause = true;
("disable", "") => unsafe {
OPTIONS.disable = true;
},

("temp-regs", _) => match opt_val.parse() {
Expand Down