diff --git a/concurrent_set.c b/concurrent_set.c index 87279384eb8893..dab7ef2180b605 100644 --- a/concurrent_set.c +++ b/concurrent_set.c @@ -1,7 +1,6 @@ #include "internal.h" #include "internal/gc.h" #include "internal/concurrent_set.h" -#include "ruby_atomic.h" #include "ruby/atomic.h" #include "vm_sync.h" @@ -109,7 +108,7 @@ static void concurrent_set_try_resize_without_locking(VALUE old_set_obj, VALUE *set_obj_ptr) { // Check if another thread has already resized. - if (RUBY_ATOMIC_VALUE_LOAD(*set_obj_ptr) != old_set_obj) { + if (rbimpl_atomic_value_load(set_obj_ptr, RBIMPL_ATOMIC_ACQUIRE) != old_set_obj) { return; } @@ -117,7 +116,7 @@ concurrent_set_try_resize_without_locking(VALUE old_set_obj, VALUE *set_obj_ptr) // This may overcount by up to the number of threads concurrently attempting to insert // GC may also happen between now and the set being rebuilt - int expected_size = RUBY_ATOMIC_LOAD(old_set->size) - old_set->deleted_entries; + int expected_size = rbimpl_atomic_load(&old_set->size, RBIMPL_ATOMIC_RELAXED) - old_set->deleted_entries; struct concurrent_set_entry *old_entries = old_set->entries; int old_capacity = old_set->capacity; @@ -135,13 +134,13 @@ concurrent_set_try_resize_without_locking(VALUE old_set_obj, VALUE *set_obj_ptr) for (int i = 0; i < old_capacity; i++) { struct concurrent_set_entry *entry = &old_entries[i]; - VALUE key = RUBY_ATOMIC_VALUE_EXCHANGE(entry->key, CONCURRENT_SET_MOVED); + VALUE key = rbimpl_atomic_value_exchange(&entry->key, CONCURRENT_SET_MOVED, RBIMPL_ATOMIC_ACQUIRE); RUBY_ASSERT(key != CONCURRENT_SET_MOVED); if (key < CONCURRENT_SET_SPECIAL_VALUE_COUNT) continue; if (!RB_SPECIAL_CONST_P(key) && rb_objspace_garbage_object_p(key)) continue; - VALUE hash = RUBY_ATOMIC_VALUE_LOAD(entry->hash); + VALUE hash = rbimpl_atomic_value_load(&entry->hash, RBIMPL_ATOMIC_RELAXED); if (hash == 0) { // Either in-progress insert or extremely unlikely 0 hash. // Re-calculate the hash. @@ -174,7 +173,7 @@ concurrent_set_try_resize_without_locking(VALUE old_set_obj, VALUE *set_obj_ptr) } } - RUBY_ATOMIC_VALUE_SET(*set_obj_ptr, new_set_obj); + rbimpl_atomic_value_store(set_obj_ptr, new_set_obj, RBIMPL_ATOMIC_RELEASE); RB_GC_GUARD(old_set_obj); } @@ -196,7 +195,7 @@ rb_concurrent_set_find(VALUE *set_obj_ptr, VALUE key) VALUE hash = 0; retry: - set_obj = RUBY_ATOMIC_VALUE_LOAD(*set_obj_ptr); + set_obj = rbimpl_atomic_value_load(set_obj_ptr, RBIMPL_ATOMIC_ACQUIRE); RUBY_ASSERT(set_obj); struct concurrent_set *set = RTYPEDDATA_GET_DATA(set_obj); @@ -212,7 +211,7 @@ rb_concurrent_set_find(VALUE *set_obj_ptr, VALUE key) while (true) { struct concurrent_set_entry *entry = &set->entries[idx]; - VALUE curr_key = RUBY_ATOMIC_VALUE_LOAD(entry->key); + VALUE curr_key = rbimpl_atomic_value_load(&entry->key, RBIMPL_ATOMIC_ACQUIRE); switch (curr_key) { case CONCURRENT_SET_EMPTY: @@ -225,13 +224,13 @@ rb_concurrent_set_find(VALUE *set_obj_ptr, VALUE key) goto retry; default: { - VALUE curr_hash = RUBY_ATOMIC_VALUE_LOAD(entry->hash); + VALUE curr_hash = rbimpl_atomic_value_load(&entry->hash, RBIMPL_ATOMIC_RELAXED); if (curr_hash != 0 && curr_hash != hash) break; if (UNLIKELY(!RB_SPECIAL_CONST_P(curr_key) && rb_objspace_garbage_object_p(curr_key))) { // This is a weakref set, so after marking but before sweeping is complete we may find a matching garbage object. // Skip it and mark it as deleted. - RUBY_ATOMIC_VALUE_CAS(entry->key, curr_key, CONCURRENT_SET_DELETED); + rbimpl_atomic_value_cas(&entry->key, curr_key, CONCURRENT_SET_DELETED, RBIMPL_ATOMIC_RELEASE, RBIMPL_ATOMIC_RELAXED); break; } @@ -259,7 +258,7 @@ rb_concurrent_set_find_or_insert(VALUE *set_obj_ptr, VALUE key, void *data) VALUE hash = 0; retry: - set_obj = RUBY_ATOMIC_VALUE_LOAD(*set_obj_ptr); + set_obj = rbimpl_atomic_value_load(set_obj_ptr, RBIMPL_ATOMIC_ACQUIRE); RUBY_ASSERT(set_obj); struct concurrent_set *set = RTYPEDDATA_GET_DATA(set_obj); @@ -275,7 +274,7 @@ rb_concurrent_set_find_or_insert(VALUE *set_obj_ptr, VALUE key, void *data) while (true) { struct concurrent_set_entry *entry = &set->entries[idx]; - VALUE curr_key = RUBY_ATOMIC_VALUE_LOAD(entry->key); + VALUE curr_key = rbimpl_atomic_value_load(&entry->key, RBIMPL_ATOMIC_ACQUIRE); switch (curr_key) { case CONCURRENT_SET_EMPTY: { @@ -286,7 +285,7 @@ rb_concurrent_set_find_or_insert(VALUE *set_obj_ptr, VALUE key, void *data) inserting = true; } - rb_atomic_t prev_size = RUBY_ATOMIC_FETCH_ADD(set->size, 1); + rb_atomic_t prev_size = rbimpl_atomic_fetch_add(&set->size, 1, RBIMPL_ATOMIC_RELAXED); if (UNLIKELY(prev_size > set->capacity / 2)) { concurrent_set_try_resize(set_obj, set_obj_ptr); @@ -294,16 +293,16 @@ rb_concurrent_set_find_or_insert(VALUE *set_obj_ptr, VALUE key, void *data) goto retry; } - curr_key = RUBY_ATOMIC_VALUE_CAS(entry->key, CONCURRENT_SET_EMPTY, key); + curr_key = rbimpl_atomic_value_cas(&entry->key, CONCURRENT_SET_EMPTY, key, RBIMPL_ATOMIC_RELEASE, RBIMPL_ATOMIC_RELAXED); if (curr_key == CONCURRENT_SET_EMPTY) { - RUBY_ATOMIC_VALUE_SET(entry->hash, hash); + rbimpl_atomic_value_store(&entry->hash, hash, RBIMPL_ATOMIC_RELAXED); RB_GC_GUARD(set_obj); return key; } else { // Entry was not inserted. - RUBY_ATOMIC_DEC(set->size); + rbimpl_atomic_sub(&set->size, 1, RBIMPL_ATOMIC_RELAXED); // Another thread won the race, try again at the same location. continue; @@ -317,13 +316,13 @@ rb_concurrent_set_find_or_insert(VALUE *set_obj_ptr, VALUE key, void *data) goto retry; default: { - VALUE curr_hash = RUBY_ATOMIC_VALUE_LOAD(entry->hash); + VALUE curr_hash = rbimpl_atomic_value_load(&entry->hash, RBIMPL_ATOMIC_RELAXED); if (curr_hash != 0 && curr_hash != hash) break; if (UNLIKELY(!RB_SPECIAL_CONST_P(curr_key) && rb_objspace_garbage_object_p(curr_key))) { // This is a weakref set, so after marking but before sweeping is complete we may find a matching garbage object. // Skip it and mark it as deleted. - RUBY_ATOMIC_VALUE_CAS(entry->key, curr_key, CONCURRENT_SET_DELETED); + rbimpl_atomic_value_cas(&entry->key, curr_key, CONCURRENT_SET_DELETED, RBIMPL_ATOMIC_RELEASE, RBIMPL_ATOMIC_RELAXED); break; } @@ -363,7 +362,7 @@ rb_concurrent_set_delete_by_identity(VALUE set_obj, VALUE key) while (true) { struct concurrent_set_entry *entry = &set->entries[idx]; - VALUE curr_key = RUBY_ATOMIC_VALUE_LOAD(entry->key); + VALUE curr_key = entry->key; switch (curr_key) { case CONCURRENT_SET_EMPTY: diff --git a/insns.def b/insns.def index 69a8210d7d6f99..8225d1cceaf97e 100644 --- a/insns.def +++ b/insns.def @@ -1598,6 +1598,7 @@ opt_succ (CALL_DATA cd) (VALUE recv) (VALUE val) +// attr bool zjit_profile = true; { val = vm_opt_succ(recv); diff --git a/zjit/bindgen/src/main.rs b/zjit/bindgen/src/main.rs index 43fec090149ef8..b54e9404fdb10e 100644 --- a/zjit/bindgen/src/main.rs +++ b/zjit/bindgen/src/main.rs @@ -73,6 +73,7 @@ fn main() { .allowlist_function("rb_utf8_str_new") .allowlist_function("rb_str_buf_append") .allowlist_function("rb_str_dup") + .allowlist_function("rb_str_getbyte") .allowlist_type("ruby_preserved_encindex") .allowlist_function("rb_class2name") diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 0bf0205b5351d2..049501dd1518fb 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -363,6 +363,7 @@ fn gen_insn(cb: &mut CodeBlock, jit: &mut JITState, asm: &mut Assembler, functio // If it happens we abort the compilation for now Insn::StringConcat { strings, state, .. } if strings.is_empty() => return Err(*state), Insn::StringConcat { strings, state } => gen_string_concat(jit, asm, opnds!(strings), &function.frame_state(*state)), + &Insn::StringGetbyteFixnum { string, index } => gen_string_getbyte_fixnum(asm, opnd!(string), opnd!(index)), Insn::StringIntern { val, state } => gen_intern(asm, opnd!(val), &function.frame_state(*state)), Insn::ToRegexp { opt, values, state } => gen_toregexp(jit, asm, *opt, opnds!(values), &function.frame_state(*state)), Insn::Param { idx } => unreachable!("block.insns should not have Insn::Param({idx})"), @@ -2109,6 +2110,11 @@ fn gen_string_concat(jit: &mut JITState, asm: &mut Assembler, strings: Vec result } +fn gen_string_getbyte_fixnum(asm: &mut Assembler, string: Opnd, index: Opnd) -> Opnd { + // TODO(max): Open-code rb_str_getbyte to avoid a call + asm_ccall!(asm, rb_str_getbyte, string, index) +} + /// Generate a JIT entry that just increments exit_compilation_failure and exits fn gen_compile_error_counter(cb: &mut CodeBlock, compile_error: &CompileError) -> Result { let mut asm = Assembler::new(); diff --git a/zjit/src/cruby.rs b/zjit/src/cruby.rs index dca4d9180556e8..5f4eac1db5ed9e 100644 --- a/zjit/src/cruby.rs +++ b/zjit/src/cruby.rs @@ -132,6 +132,7 @@ unsafe extern "C" { pub fn rb_hash_empty_p(hash: VALUE) -> VALUE; pub fn rb_yjit_str_concat_codepoint(str: VALUE, codepoint: VALUE); pub fn rb_str_setbyte(str: VALUE, index: VALUE, value: VALUE) -> VALUE; + pub fn rb_str_getbyte(str: VALUE, index: VALUE) -> VALUE; pub fn rb_vm_splat_array(flag: VALUE, ary: VALUE) -> VALUE; pub fn rb_vm_concat_array(ary1: VALUE, ary2st: VALUE) -> VALUE; pub fn rb_vm_get_special_object(reg_ep: *const VALUE, value_type: vm_special_object_type) -> VALUE; diff --git a/zjit/src/cruby_bindings.inc.rs b/zjit/src/cruby_bindings.inc.rs index 17cda12a0b6204..ffaafed5ff33c2 100644 --- a/zjit/src/cruby_bindings.inc.rs +++ b/zjit/src/cruby_bindings.inc.rs @@ -702,9 +702,10 @@ pub const YARVINSN_zjit_opt_aset: ruby_vminsn_type = 237; pub const YARVINSN_zjit_opt_length: ruby_vminsn_type = 238; pub const YARVINSN_zjit_opt_size: ruby_vminsn_type = 239; pub const YARVINSN_zjit_opt_empty_p: ruby_vminsn_type = 240; -pub const YARVINSN_zjit_opt_not: ruby_vminsn_type = 241; -pub const YARVINSN_zjit_opt_regexpmatch2: ruby_vminsn_type = 242; -pub const VM_INSTRUCTION_SIZE: ruby_vminsn_type = 243; +pub const YARVINSN_zjit_opt_succ: ruby_vminsn_type = 241; +pub const YARVINSN_zjit_opt_not: ruby_vminsn_type = 242; +pub const YARVINSN_zjit_opt_regexpmatch2: ruby_vminsn_type = 243; +pub const VM_INSTRUCTION_SIZE: ruby_vminsn_type = 244; pub type ruby_vminsn_type = u32; pub type rb_iseq_callback = ::std::option::Option< unsafe extern "C" fn(arg1: *const rb_iseq_t, arg2: *mut ::std::os::raw::c_void), diff --git a/zjit/src/cruby_methods.rs b/zjit/src/cruby_methods.rs index 27e6b151bf5020..11496f7b9874a0 100644 --- a/zjit/src/cruby_methods.rs +++ b/zjit/src/cruby_methods.rs @@ -190,6 +190,7 @@ pub fn init() -> Annotations { annotate!(rb_mKernel, "itself", inline_kernel_itself); annotate!(rb_cString, "bytesize", types::Fixnum, no_gc, leaf); annotate!(rb_cString, "to_s", types::StringExact); + annotate!(rb_cString, "getbyte", inline_string_getbyte); annotate!(rb_cModule, "name", types::StringExact.union(types::NilClass), no_gc, leaf, elidable); annotate!(rb_cModule, "===", types::BoolExact, no_gc, leaf); annotate!(rb_cArray, "length", types::Fixnum, no_gc, leaf, elidable); @@ -205,6 +206,7 @@ pub fn init() -> Annotations { annotate!(rb_cBasicObject, "==", types::BoolExact, no_gc, leaf, elidable); annotate!(rb_cBasicObject, "!", types::BoolExact, no_gc, leaf, elidable); annotate!(rb_cBasicObject, "initialize", types::NilClass, no_gc, leaf, elidable); + annotate!(rb_cInteger, "succ", inline_integer_succ); annotate!(rb_cString, "to_s", inline_string_to_s); let thread_singleton = unsafe { rb_singleton_class(rb_cThread) }; annotate!(thread_singleton, "current", types::BasicObject, no_gc, leaf); @@ -257,3 +259,26 @@ fn inline_hash_aref(fun: &mut hir::Function, block: hir::BlockId, recv: hir::Ins } None } + +fn inline_string_getbyte(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], state: hir::InsnId) -> Option { + let &[index] = args else { return None; }; + if fun.likely_a(index, types::Fixnum, state) { + // String#getbyte with a Fixnum is leaf and nogc; otherwise it may run arbitrary Ruby code + // when converting the index to a C integer. + let index = fun.coerce_to(block, index, types::Fixnum, state); + let result = fun.push_insn(block, hir::Insn::StringGetbyteFixnum { string: recv, index }); + return Some(result); + } + None +} + +fn inline_integer_succ(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], state: hir::InsnId) -> Option { + if !args.is_empty() { return None; } + if fun.likely_a(recv, types::Fixnum, state) { + let left = fun.coerce_to(block, recv, types::Fixnum, state); + let right = fun.push_insn(block, hir::Insn::Const { val: hir::Const::Value(VALUE::fixnum_from_usize(1)) }); + let result = fun.push_insn(block, hir::Insn::FixnumAdd { left, right, state }); + return Some(result); + } + None +} diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 4ad20b9c1d8295..a87a416e9e5408 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -557,6 +557,8 @@ pub enum Insn { StringCopy { val: InsnId, chilled: bool, state: InsnId }, StringIntern { val: InsnId, state: InsnId }, StringConcat { strings: Vec, state: InsnId }, + /// Call rb_str_getbyte with known-Fixnum index + StringGetbyteFixnum { string: InsnId, index: InsnId }, /// Combine count stack values into a regexp ToRegexp { opt: usize, values: Vec, state: InsnId }, @@ -860,6 +862,7 @@ impl Insn { // but we don't have type information here in `impl Insn`. See rb_range_new(). Insn::NewRange { .. } => true, Insn::NewRangeFixnum { .. } => false, + Insn::StringGetbyteFixnum { .. } => false, _ => true, } } @@ -941,6 +944,9 @@ impl<'a> std::fmt::Display for InsnPrinter<'a> { Ok(()) } + Insn::StringGetbyteFixnum { string, index, .. } => { + write!(f, "StringGetbyteFixnum {string}, {index}") + } Insn::ToRegexp { values, opt, .. } => { write!(f, "ToRegexp")?; let mut prefix = " "; @@ -1498,6 +1504,7 @@ impl Function { &StringCopy { val, chilled, state } => StringCopy { val: find!(val), chilled, state }, &StringIntern { val, state } => StringIntern { val: find!(val), state: find!(state) }, &StringConcat { ref strings, state } => StringConcat { strings: find_vec!(strings), state: find!(state) }, + &StringGetbyteFixnum { string, index } => StringGetbyteFixnum { string: find!(string), index: find!(index) }, &ToRegexp { opt, ref values, state } => ToRegexp { opt, values: find_vec!(values), state }, &Test { val } => Test { val: find!(val) }, &IsNil { val } => IsNil { val: find!(val) }, @@ -1679,6 +1686,7 @@ impl Function { Insn::StringCopy { .. } => types::StringExact, Insn::StringIntern { .. } => types::Symbol, Insn::StringConcat { .. } => types::StringExact, + Insn::StringGetbyteFixnum { .. } => types::Fixnum.union(types::NilClass), Insn::ToRegexp { .. } => types::RegexpExact, Insn::NewArray { .. } => types::ArrayExact, Insn::ArrayDup { .. } => types::ArrayExact, @@ -2712,6 +2720,10 @@ impl Function { worklist.extend(strings); worklist.push_back(state); } + &Insn::StringGetbyteFixnum { string, index } => { + worklist.push_back(string); + worklist.push_back(index); + } &Insn::ToRegexp { ref values, state, .. } => { worklist.extend(values); worklist.push_back(state); @@ -13122,4 +13134,139 @@ mod opt_tests { Return v27 "); } + + #[test] + fn test_optimize_string_getbyte_fixnum() { + eval(r#" + def test(s, i) = s.getbyte(i) + test("foo", 0) + "#); + assert_snapshot!(hir_string("test"), @r" + fn test@:2: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@5 + v3:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2, v3) + bb1(v6:BasicObject, v7:BasicObject, v8:BasicObject): + EntryPoint JIT(0) + Jump bb2(v6, v7, v8) + bb2(v10:BasicObject, v11:BasicObject, v12:BasicObject): + PatchPoint MethodRedefined(String@0x1000, getbyte@0x1008, cme:0x1010) + PatchPoint NoSingletonClass(String@0x1000) + v26:StringExact = GuardType v11, StringExact + v27:Fixnum = GuardType v12, Fixnum + v28:NilClass|Fixnum = StringGetbyteFixnum v26, v27 + CheckInterrupts + Return v28 + "); + } + + #[test] + fn test_elide_string_getbyte_fixnum() { + eval(r#" + def test(s, i) + s.getbyte(i) + 5 + end + test("foo", 0) + "#); + assert_snapshot!(hir_string("test"), @r" + fn test@:3: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@5 + v3:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2, v3) + bb1(v6:BasicObject, v7:BasicObject, v8:BasicObject): + EntryPoint JIT(0) + Jump bb2(v6, v7, v8) + bb2(v10:BasicObject, v11:BasicObject, v12:BasicObject): + PatchPoint MethodRedefined(String@0x1000, getbyte@0x1008, cme:0x1010) + PatchPoint NoSingletonClass(String@0x1000) + v29:StringExact = GuardType v11, StringExact + v30:Fixnum = GuardType v12, Fixnum + v20:Fixnum[5] = Const Value(5) + CheckInterrupts + Return v20 + "); + } + + #[test] + fn test_inline_integer_succ_with_fixnum() { + eval(" + def test(x) = x.succ + test(4) + "); + assert_contains_opcode("test", YARVINSN_opt_succ); + assert_snapshot!(hir_string("test"), @r" + fn test@:2: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2) + bb1(v5:BasicObject, v6:BasicObject): + EntryPoint JIT(0) + Jump bb2(v5, v6) + bb2(v8:BasicObject, v9:BasicObject): + PatchPoint MethodRedefined(Integer@0x1000, succ@0x1008, cme:0x1010) + v24:Fixnum = GuardType v9, Fixnum + v25:Fixnum[1] = Const Value(1) + v26:Fixnum = FixnumAdd v24, v25 + CheckInterrupts + Return v26 + "); + } + + #[test] + fn test_dont_inline_integer_succ_with_bignum() { + eval(" + def test(x) = x.succ + test(4 << 70) + "); + assert_contains_opcode("test", YARVINSN_opt_succ); + assert_snapshot!(hir_string("test"), @r" + fn test@:2: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2) + bb1(v5:BasicObject, v6:BasicObject): + EntryPoint JIT(0) + Jump bb2(v5, v6) + bb2(v8:BasicObject, v9:BasicObject): + PatchPoint MethodRedefined(Integer@0x1000, succ@0x1008, cme:0x1010) + v24:Integer = GuardType v9, Integer + v25:BasicObject = CCallWithFrame succ@0x1038, v24 + CheckInterrupts + Return v25 + "); + } + + #[test] + fn test_dont_inline_integer_succ_with_args() { + eval(" + def test = 4.succ 1 + "); + assert_snapshot!(hir_string("test"), @r" + fn test@:2: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + Jump bb2(v1) + bb1(v4:BasicObject): + EntryPoint JIT(0) + Jump bb2(v4) + bb2(v6:BasicObject): + v10:Fixnum[4] = Const Value(4) + v11:Fixnum[1] = Const Value(1) + v13:BasicObject = SendWithoutBlock v10, :succ, v11 + CheckInterrupts + Return v13 + "); + } } diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index e7db47142bcf65..e935ec9731f383 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -82,6 +82,7 @@ fn profile_insn(bare_opcode: ruby_vminsn_type, ec: EcPtr) { YARVINSN_objtostring => profile_operands(profiler, profile, 1), YARVINSN_opt_length => profile_operands(profiler, profile, 1), YARVINSN_opt_size => profile_operands(profiler, profile, 1), + YARVINSN_opt_succ => profile_operands(profiler, profile, 1), YARVINSN_opt_send_without_block => { let cd: *const rb_call_data = profiler.insn_opnd(0).as_ptr(); let argc = unsafe { vm_ci_argc((*cd).ci) };