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

Audit mt.rs #165

Merged
merged 4 commits into from
Dec 4, 2020
+28 −12
Merged

Audit mt.rs #165

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97921db
Don't leak memory if we race with another thread when starting tracing.
ltratt Dec 4, 2020
f455452
'mem::forget' is a more idiomatic way of not dropping something.
ltratt Dec 4, 2020
56d0156
Ensure the hot count is incremented even if we're racing with another…
ltratt Dec 4, 2020
ec92144
Pointer tagging should be OR not left-shift.
ltratt Dec 4, 2020
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
40 changes: 28 additions & 12 deletions ykrt/src/mt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,10 @@ impl MTThread {
loop {
// We need Acquire ordering, as PHASE_COMPILED will need to read information written to
// external data as a result of the PHASE_TRACING -> PHASE_COMPILED transition.
let lp = loc.pack.load(Ordering::Acquire);
let mut lp = loc.pack.load(Ordering::Acquire);
match lp & PHASE_TAG {
PHASE_COUNTING => {
let count = (lp & !PHASE_TAG) >> 2;
let mut count = (lp & !PHASE_TAG) >> 2;
if count >= self.inner.hot_threshold {
if self.inner.tracer.is_some() {
// This thread is already tracing. Note that we don't increment the hot
Expand All @@ -244,11 +244,24 @@ impl MTThread {
Rc::get_mut(&mut self.inner).unwrap().tracer =
Some((start_tracing(self.inner.tracing_kind), loc_id));
return None;
} else {
// We raced with another thread that's also trying to trace this
// Location, so free the malloc'd block.
unsafe { Box::from_raw(loc_id) };
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should just eagerly allocate each thread an ID upon it's creation. Then the ID can be reused across many tracing sessions at zero additional cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Once the current bug is fixed I'll look into that.

Copy link
Contributor

Choose a reason for hiding this comment

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

And by extension, is the address of the MTInner a better ID?

Copy link
Contributor Author

@ltratt ltratt Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That feature might be disappearing at some point. Not sure yet.

}
} else {
let new_pack = PHASE_COUNTING | ((count + 1) << PHASE_NUM_BITS);
if loc.pack.compare_and_swap(lp, new_pack, Ordering::Release) == lp {
return None;
loop {
let new_pack = PHASE_COUNTING | ((count + 1) << PHASE_NUM_BITS);
if loc.pack.compare_and_swap(lp, new_pack, Ordering::Release) == lp {
return None;
}
// We raced with another thread, but we'd still like to try
// incrementing the count if possible, so we try again.
lp = loc.pack.load(Ordering::Acquire);
count = (lp & !PHASE_TAG) >> 2;
if count >= self.inner.hot_threshold {
break;
}
}
}
}
Expand Down Expand Up @@ -283,20 +296,23 @@ impl MTThread {
// FIXME: free up the memory when the trace is no longer used.
let ptr = Box::into_raw(Box::new(ct)) as usize;

let new_pack = PHASE_COMPILED | (ptr << PHASE_NUM_BITS);
let new_pack = ptr | PHASE_COMPILED;
loc.pack.store(new_pack, Ordering::Release);
// Free the small block of memory we used as a Location ID.
unsafe { Box::from_raw(loc_id) };
Rc::get_mut(&mut self.inner).unwrap().tracer = None;
return None;
}
PHASE_COMPILED => {
let ptr = (lp >> PHASE_NUM_BITS) as *const u8;
let bct = unsafe { Box::from_raw(ptr as *mut CompiledTrace<I>) };
let tptr = bct.ptr();
let func: fn(&mut I) -> bool = unsafe { mem::transmute(tptr) };
let _raw = Box::into_raw(bct);
return Some(func);
let p = (lp & !PHASE_TAG) as *const ();
// Retrieve the CompiledTrace to gain access to the memory containing the
// trace.
let bct = unsafe { Box::from_raw(p as *mut CompiledTrace<I>) };
let f = unsafe { mem::transmute::<_, fn(&mut I) -> bool>(bct.ptr()) };
// Forget the CompiledTrace again to make sure it isn't dropped before we had a
// chance to execute it.
mem::forget(bct);
return Some(f);
}
_ => unreachable!(),
}
Expand Down