Skip to content

ZJIT: Refine singleton class invariant to only invalidate on method shadowing #950

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

Draft
tekknolagi wants to merge 1 commit into
base: ruby-upstream
Choose a base branch
from
Draft

ZJIT: Refine singleton class invariant to only invalidate on method shadowing #950

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion class.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,7 +1422,6 @@ make_singleton_class(VALUE obj)
RBASIC_SET_CLASS(obj, klass);
rb_singleton_class_attached(klass, obj);
rb_yjit_invalidate_no_singleton_class(orig_class);
rb_zjit_invalidate_no_singleton_class(orig_class);

SET_METACLASS_OF(klass, METACLASS_OF(rb_class_real(orig_class)));
return klass;
Expand Down
9 changes: 9 additions & 0 deletions vm_method.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1500,6 +1500,15 @@ rb_method_entry_make(VALUE klass, ID mid, VALUE defined_class, rb_method_visibil
check_override_opt_method(klass, (VALUE)mid);
}

// If a method was added to a singleton class that shadows a method on
// the original class, invalidate JIT code that assumes no shadowing.
if (RCLASS_SINGLETON_P(orig_klass)) {
VALUE super_klass = RCLASS_SUPER(orig_klass);
if (rb_method_entry(super_klass, mid)) {
rb_zjit_invalidate_singleton_class_has_shadowing_method(super_klass);
}
}

return me;
}

Expand Down
4 changes: 2 additions & 2 deletions zjit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ void rb_zjit_iseq_update_references(void *payload);
void rb_zjit_iseq_free(const rb_iseq_t *iseq);
void rb_zjit_before_ractor_spawn(void);
void rb_zjit_tracing_invalidate_all(void);
void rb_zjit_invalidate_no_singleton_class(VALUE klass);
void rb_zjit_invalidate_singleton_class_has_shadowing_method(VALUE klass);
#else
#define rb_zjit_entry 0
static inline void rb_zjit_compile_iseq(const rb_iseq_t *iseq, bool jit_exception) {}
Expand All @@ -39,7 +39,7 @@ static inline void rb_zjit_invalidate_no_ep_escape(const rb_iseq_t *iseq) {}
static inline void rb_zjit_constant_state_changed(ID id) {}
static inline void rb_zjit_before_ractor_spawn(void) {}
static inline void rb_zjit_tracing_invalidate_all(void) {}
static inline void rb_zjit_invalidate_no_singleton_class(VALUE klass) {}
static inline void rb_zjit_invalidate_singleton_class_has_shadowing_method(VALUE klass) {}
#endif // #if USE_ZJIT

#define rb_zjit_enabled_p (rb_zjit_entry != 0)
Expand Down
6 changes: 3 additions & 3 deletions zjit/src/codegen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::slice;
use crate::backend::current::ALLOC_REGS;
use crate::invariants::{
track_bop_assumption, track_cme_assumption, track_no_ep_escape_assumption, track_no_trace_point_assumption,
track_single_ractor_assumption, track_stable_constant_names_assumption, track_no_singleton_class_assumption
track_single_ractor_assumption, track_stable_constant_names_assumption, track_no_singleton_class_shadowing_assumption
};
use crate::gc::append_gc_offsets;
use crate::payload::{get_or_create_iseq_payload, IseqCodePtrs, IseqVersion, IseqVersionRef, IseqStatus};
Expand Down Expand Up @@ -904,8 +904,8 @@ pub fn split_patch_point(asm: &mut Assembler, target: &Target, invariant: Invari
Invariant::SingleRactorMode => {
track_single_ractor_assumption(code_ptr, side_exit_ptr, version);
}
Invariant::NoSingletonClass { klass } => {
track_no_singleton_class_assumption(klass, code_ptr, side_exit_ptr, version);
Invariant::NoSingletonClassWithShadowingMethod { klass } => {
track_no_singleton_class_shadowing_assumption(klass, code_ptr, side_exit_ptr, version);
}
}
});
Expand Down
21 changes: 21 additions & 0 deletions zjit/src/codegen_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4542,6 +4542,27 @@ fn test_singleton_class_invalidation_optimized_variadic_ccall() {
"), @"[1, 1000]");
}

#[test]
fn test_singleton_class_shadowing_method_invalidation() {
// Profile String#length so it gets JIT-compiled, then define a singleton
// method that shadows it, then call and verify the singleton method is used.
assert_snapshot!(inspect("
def test(s)
s.length
end

results = []
results << test('asdf')

special = 'world'
def special.length = 99

results << test(special)
results << test('hello')
results
"), @"[4, 99, 5]");
}

#[test]
fn test_is_a_string_special_case() {
assert_snapshot!(inspect(r#"
Expand Down
2 changes: 1 addition & 1 deletion zjit/src/cruby_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,7 @@ fn inline_kernel_respond_to_p(
(_, _) => return None, // not public and include_all not known, can't compile
};
// Check singleton class assumption first, before emitting other patchpoints
if !fun.assume_no_singleton_classes(block, recv_class, state) {
if !fun.assume_no_singleton_class_method_shadowing(block, recv_class, state) {
return None;
}
fun.push_insn(block, hir::Insn::PatchPoint { invariant: hir::Invariant::NoTracePoint, state });
Expand Down
78 changes: 39 additions & 39 deletions zjit/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#![allow(clippy::match_like_matches_macro)]
use crate::{
backend::lir::C_ARG_OPNDS,
cast::IntoUsize, codegen::local_idx_to_ep_offset, cruby::*, invariants::has_singleton_class_of, payload::{get_or_create_iseq_payload, IseqPayload}, options::{debug, get_option, DumpHIR}, state::ZJITState, json::Json
cast::IntoUsize, codegen::local_idx_to_ep_offset, cruby::*, invariants::has_singleton_class_method_shadowing, payload::{get_or_create_iseq_payload, IseqPayload}, options::{debug, get_option, DumpHIR}, state::ZJITState, json::Json
};
use std::{
cell::RefCell, collections::{BTreeSet, HashMap, HashSet, VecDeque}, ffi::{c_void, c_uint, c_int, CStr}, fmt::Display, mem::{align_of, size_of}, ptr, slice::Iter
Expand Down Expand Up @@ -146,9 +146,9 @@ pub enum Invariant {
NoEPEscape(IseqPtr),
/// There is one ractor running. If a non-root ractor gets spawned, this is invalidated.
SingleRactorMode,
/// Objects of this class have no singleton class.
/// When a singleton class is created for an object of this class, this is invalidated.
NoSingletonClass {
/// No singleton class of an instance of this class has a method that shadows a method
/// on this class. When such a shadowing method is defined, this is invalidated.
NoSingletonClassWithShadowingMethod {
klass: VALUE,
},
}
Expand Down Expand Up @@ -284,9 +284,9 @@ impl<'a> std::fmt::Display for InvariantPrinter<'a> {
Invariant::NoTracePoint => write!(f, "NoTracePoint"),
Invariant::NoEPEscape(iseq) => write!(f, "NoEPEscape({})", &iseq_name(iseq)),
Invariant::SingleRactorMode => write!(f, "SingleRactorMode"),
Invariant::NoSingletonClass { klass } => {
Invariant::NoSingletonClassWithShadowingMethod { klass } => {
let class_name = get_class_name(klass);
write!(f, "NoSingletonClass({}@{:p})",
write!(f, "NoSingletonClassWithShadowingMethod({}@{:p})",
class_name,
self.ptr_map.map_ptr(klass.as_ptr::<VALUE>()))
}
Expand Down Expand Up @@ -660,9 +660,9 @@ pub enum SendFallbackReason {
ComplexArgPass,
/// Caller has keyword arguments but callee doesn't expect them; need to convert to hash.
UnexpectedKeywordArgs,
/// A singleton class has been seen for the receiver class, so we skip the optimization
/// to avoid an invalidation loop.
SingletonClassSeen,
/// A singleton class with a shadowing method has been seen for the receiver class,
/// so we skip the optimization to avoid an invalidation loop.
SingletonClassWithShadowingMethodSeen,
/// The super call is passed a block that the optimizer does not support.
SuperCallWithBlock,
/// When the `super` is in a block, finding the running CME for guarding requires a loop. Not
Expand Down Expand Up @@ -719,7 +719,7 @@ impl Display for SendFallbackReason {
ArgcParamMismatch => write!(f, "Argument count does not match parameter count"),
ComplexArgPass => write!(f, "Complex argument passing"),
UnexpectedKeywordArgs => write!(f, "Unexpected Keyword Args"),
SingletonClassSeen => write!(f, "Singleton class previously created for receiver class"),
SingletonClassWithShadowingMethodSeen => write!(f, "Singleton class with shadowing method previously seen for receiver class"),
SuperFromBlock => write!(f, "super: call from within a block"),
SuperCallWithBlock => write!(f, "super: call made with a block"),
SuperClassNotFound => write!(f, "super: profiled class cannot be found"),
Expand Down Expand Up @@ -2169,21 +2169,21 @@ impl Function {
}
}

/// Assume that objects of a given class will have no singleton class.
/// Returns true if safe to assume so and emits a PatchPoint.
/// Returns false if we've already seen a singleton class for this class,
/// to avoid an invalidation loop.
pub fn assume_no_singleton_classes(&mut self, block: BlockId, klass: VALUE, state: InsnId) -> bool {
/// Assume that no singleton class of an instance of a given class will have a method
/// that shadows a method on the class. Returns true if safe to assume so and emits a
/// PatchPoint. Returns false if we've already seen such shadowing, to avoid an
/// invalidation loop.
pub fn assume_no_singleton_class_method_shadowing(&mut self, block: BlockId, klass: VALUE, state: InsnId) -> bool {
if !klass.instance_can_have_singleton_class() {
// This class can never have a singleton class, so no patchpoint needed.
return true;
}
if has_singleton_class_of(klass) {
// We've seen a singleton class for this klass. Disable the optimization
// to avoid an invalidation loop.
if has_singleton_class_method_shadowing(klass) {
// We've seen a singleton class with a shadowing method for this klass.
// Disable the optimization to avoid an invalidation loop.
return false;
}
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoSingletonClass { klass }, state });
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoSingletonClassWithShadowingMethod { klass }, state });
true
}

Expand Down Expand Up @@ -2980,7 +2980,7 @@ impl Function {
return false;
}
self.gen_patch_points_for_optimized_ccall(block, class, method_id, cme, state);
if !self.assume_no_singleton_classes(block, class, state) {
if !self.assume_no_singleton_class_method_shadowing(block, class, state) {
return false;
}
true
Expand Down Expand Up @@ -3199,8 +3199,8 @@ impl Function {
}

// Check singleton class assumption first, before emitting other patchpoints
if !self.assume_no_singleton_classes(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassSeen);
if !self.assume_no_singleton_class_method_shadowing(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassWithShadowingMethodSeen);
self.push_insn_id(block, insn_id); continue;
}

Expand Down Expand Up @@ -3243,8 +3243,8 @@ impl Function {
self.push_insn_id(block, insn_id); continue;
}
// Check singleton class assumption first, before emitting other patchpoints
if !self.assume_no_singleton_classes(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassSeen);
if !self.assume_no_singleton_class_method_shadowing(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassWithShadowingMethodSeen);
self.push_insn_id(block, insn_id); continue;
}
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state });
Expand All @@ -3267,8 +3267,8 @@ impl Function {
self.push_insn_id(block, insn_id); continue;
}
// Check singleton class assumption first, before emitting other patchpoints
if !self.assume_no_singleton_classes(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassSeen);
if !self.assume_no_singleton_class_method_shadowing(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassWithShadowingMethodSeen);
self.push_insn_id(block, insn_id); continue;
}

Expand Down Expand Up @@ -3305,8 +3305,8 @@ impl Function {
self.push_insn_id(block, insn_id); continue;
}
// Check singleton class assumption first, before emitting other patchpoints
if !self.assume_no_singleton_classes(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassSeen);
if !self.assume_no_singleton_class_method_shadowing(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassWithShadowingMethodSeen);
self.push_insn_id(block, insn_id); continue;
}
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state });
Expand Down Expand Up @@ -3340,8 +3340,8 @@ impl Function {
self.push_insn_id(block, insn_id); continue;
};
// Check singleton class assumption first, before emitting other patchpoints
if !self.assume_no_singleton_classes(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassSeen);
if !self.assume_no_singleton_class_method_shadowing(block, klass, state) {
self.set_dynamic_send_reason(insn_id, SingletonClassWithShadowingMethodSeen);
self.push_insn_id(block, insn_id); continue;
}
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state });
Expand Down Expand Up @@ -3416,7 +3416,7 @@ impl Function {
};

if recv_type.is_string() {
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoSingletonClass { klass: recv_type.class() }, state });
self.push_insn(block, Insn::PatchPoint { invariant: Invariant::NoSingletonClassWithShadowingMethod { klass: recv_type.class() }, state });
let guard = self.push_insn(block, Insn::GuardType { val, guard_type: types::String, state });
// Infer type so AnyToString can fold off this
self.insn_types[guard.0] = self.infer_type(guard);
Expand Down Expand Up @@ -4133,8 +4133,8 @@ impl Function {
}

// Check singleton class assumption first, before emitting other patchpoints
if !fun.assume_no_singleton_classes(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassSeen);
if !fun.assume_no_singleton_class_method_shadowing(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassWithShadowingMethodSeen);
return Err(());
}

Expand Down Expand Up @@ -4172,8 +4172,8 @@ impl Function {
// func(int argc, VALUE *argv, VALUE recv)

// Check singleton class assumption first, before emitting other patchpoints
if !fun.assume_no_singleton_classes(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassSeen);
if !fun.assume_no_singleton_class_method_shadowing(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassWithShadowingMethodSeen);
return Err(());
}

Expand Down Expand Up @@ -4290,8 +4290,8 @@ impl Function {
}

// Check singleton class assumption first, before emitting other patchpoints
if !fun.assume_no_singleton_classes(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassSeen);
if !fun.assume_no_singleton_class_method_shadowing(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassWithShadowingMethodSeen);
return Err(());
}

Expand Down Expand Up @@ -4372,8 +4372,8 @@ impl Function {
return Err(());
} else {
// Check singleton class assumption first, before emitting other patchpoints
if !fun.assume_no_singleton_classes(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassSeen);
if !fun.assume_no_singleton_class_method_shadowing(block, recv_class, state) {
fun.set_dynamic_send_reason(send_insn_id, SingletonClassWithShadowingMethodSeen);
return Err(());
}

Expand Down
Loading
Loading