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

Pass around the root trace rather than fishing it out of HotLocationKind. #1464

Merged
merged 1 commit into from
Nov 13, 2024
+38 −28
Merged

Pass around the root trace rather than fishing it out of HotLocationKind. #1464

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
26 changes: 7 additions & 19 deletions ykrt/src/compile/jitc_yk/codegen/x64/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use crate::{
},
CompiledTrace, Guard, GuardIdx, SideTraceInfo,
},
location::{HotLocation, HotLocationKind},
location::HotLocation,
mt::MT,
};
use dynasmrt::{
Expand Down Expand Up @@ -2397,7 +2397,12 @@ impl CompiledTrace for X64CompiledTrace {
self.buf.ptr(AssemblyOffset(0)) as *const libc::c_void
}

fn sidetraceinfo(&self, gidx: GuardIdx) -> Arc<dyn SideTraceInfo> {
fn sidetraceinfo(
&self,
root_ctr: Arc<dyn CompiledTrace>,
gidx: GuardIdx,
) -> Arc<dyn SideTraceInfo> {
let root_ctr = root_ctr.as_any().downcast::<X64CompiledTrace>().unwrap();
// FIXME: Can we reference these instead of copying them, e.g. by passing in a reference to
// the `CompiledTrace` and `gidx` or better a reference to `DeoptInfo`?
let deoptinfo = &self.deoptinfo[&usize::from(gidx)];
Expand All @@ -2408,23 +2413,6 @@ impl CompiledTrace for X64CompiledTrace {
.collect();
let callframes = deoptinfo.inlined_frames.clone();

// Get the root trace from the HotLocation.
// FIXME: It might be better if we had a consistent mechanism for doing this rather than
// fishing it out of the HotLocationKind`.
let hlarc = self.hl.upgrade().unwrap();
let hl = hlarc.lock();
let root_ctr = match &hl.kind {
HotLocationKind::Compiled(root_ctr) => Arc::clone(root_ctr)
.as_any()
.downcast::<X64CompiledTrace>()
.unwrap(),
HotLocationKind::SideTracing { root_ctr, .. } => Arc::clone(root_ctr)
.as_any()
.downcast::<X64CompiledTrace>()
.unwrap(),
_ => panic!("Unexpected HotLocationKind"),
};

// Calculate the address inside the root trace we want side-traces to jump to. Currently
// this is directly after the prologue. Later we will change this to jump to after the
// preamble and before the peeled loop.
Expand Down
18 changes: 15 additions & 3 deletions ykrt/src/compile/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ pub(crate) trait CompiledTrace: fmt::Debug + Send + Sync {
/// upcasting in Rust is incomplete.
fn as_any(self: Arc<Self>) -> Arc<dyn std::any::Any + Send + Sync + 'static>;

fn sidetraceinfo(&self, gidx: GuardIdx) -> Arc<dyn SideTraceInfo>;
fn sidetraceinfo(
&self,
root_ctr: Arc<dyn CompiledTrace>,
gidx: GuardIdx,
) -> Arc<dyn SideTraceInfo>;

/// Return a reference to the guard `id`.
fn guard(&self, gidx: GuardIdx) -> &Guard;
Expand Down Expand Up @@ -113,7 +117,11 @@ mod compiled_trace_testing {
panic!();
}

fn sidetraceinfo(&self, _gidx: GuardIdx) -> Arc<dyn SideTraceInfo> {
fn sidetraceinfo(
&self,
_root_ctr: Arc<dyn CompiledTrace>,
_gidx: GuardIdx,
) -> Arc<dyn SideTraceInfo> {
panic!();
}

Expand Down Expand Up @@ -156,7 +164,11 @@ mod compiled_trace_testing {
panic!();
}

fn sidetraceinfo(&self, _gidx: GuardIdx) -> Arc<dyn SideTraceInfo> {
fn sidetraceinfo(
&self,
_root_ctr: Arc<dyn CompiledTrace>,
_gidx: GuardIdx,
) -> Arc<dyn SideTraceInfo> {
panic!();
}

Expand Down
22 changes: 16 additions & 6 deletions ykrt/src/mt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ impl MT {
self: &Arc<Self>,
trace_iter: (Box<dyn AOTTraceIterator>, Box<[u8]>),
hl_arc: Arc<Mutex<HotLocation>>,
sidetrace: Option<(GuardIdx, Arc<dyn CompiledTrace>)>,
sidetrace: Option<(Arc<dyn CompiledTrace>, GuardIdx, Arc<dyn CompiledTrace>)>,
) {
self.stats.trace_recorded_ok();
let mt = Arc::clone(self);
Expand All @@ -271,8 +271,11 @@ impl MT {
Arc::clone(&*lk)
};
mt.stats.timing_state(TimingState::Compiling);
let (sti, guardid) = if let Some((guardid, ctr)) = &sidetrace {
(Some(ctr.sidetraceinfo(*guardid)), Some(*guardid))
let (sti, guardid) = if let Some((root_ctr, guardid, ctr)) = &sidetrace {
(
Some(ctr.sidetraceinfo(Arc::clone(root_ctr), *guardid)),
Some(*guardid),
)
} else {
(None, None)
};
Expand All @@ -287,7 +290,7 @@ impl MT {
trace_iter.1,
) {
Ok(ct) => {
if let Some((_, parent_ctr)) = sidetrace {
if let Some((_root_ctr, _, parent_ctr)) = sidetrace {
parent_ctr.guard(guardid.unwrap()).set_ctr(ct);
} else {
let mut hl = hl_arc.lock();
Expand Down Expand Up @@ -416,6 +419,7 @@ impl MT {
TransitionControlPoint::StopSideTracing {
gidx: guardid,
parent_ctr,
root_ctr,
} => {
// Assuming no bugs elsewhere, the `unwrap`s cannot fail, because
// `StartSideTracing` will have put a `Some` in the `Rc`.
Expand All @@ -438,7 +442,7 @@ impl MT {
self.queue_compile_job(
(utrace, promotions.into_boxed_slice()),
hl,
Some((guardid, parent_ctr)),
Some((root_ctr, guardid, parent_ctr)),
);
}
Err(e) => {
Expand Down Expand Up @@ -572,9 +576,14 @@ impl MT {
// ...and it's this location: we have therefore finished
// tracing the loop.
let parent_ctr = Arc::clone(parent_ctr);
let root_ctr_cl = Arc::clone(root_ctr);
lk.kind = HotLocationKind::Compiled(Arc::clone(root_ctr));
drop(lk);
TransitionControlPoint::StopSideTracing { gidx, parent_ctr }
TransitionControlPoint::StopSideTracing {
gidx,
parent_ctr,
root_ctr: root_ctr_cl,
}
}
}
_ => {
Expand Down Expand Up @@ -899,6 +908,7 @@ enum TransitionControlPoint {
StopSideTracing {
gidx: GuardIdx,
parent_ctr: Arc<dyn CompiledTrace>,
root_ctr: Arc<dyn CompiledTrace>,
},
}

Expand Down