-
Notifications
You must be signed in to change notification settings - Fork 110
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.
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
Improve logging #91
Improve logging #91
Conversation
This adds a bunch of prints whenever This is only enabled when What do you think? |
Fixed the issue spotted by |
Rebased on the current master and force-pushed again. |
@nkaretnikov what do you think about using a logging library like |
Also, what do you think about including a prefix such as the name of the test (which, IIRC, should be the name of the thread the test is running on)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some notes. In general, I think it would be very helpful to provide some kind of identifier for the various objects in loom that now emit log messages. This would allow users to track which object these messages refer to.
src/rt/alloc.rs
Outdated
@@ -15,6 +15,10 @@ pub(super) struct State { | |||
/// Track a raw allocation | |||
pub(crate) fn alloc(ptr: *mut u8) { | |||
rt::execution(|execution| { | |||
if execution.log { | |||
println!("alloc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take it or leave it, but what do you think of
println!("alloc"); | |
println!("alloc {:p}", ptr); |
using the fmt::Pointer
formatter? Although printing the memory location doesn't provide a lot of information on its own, if we print it in dealloc
as well, users can at least match pairs of alloc
and dealloc
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/alloc.rs
Outdated
); | ||
let allocation = rt::execution(|execution| { | ||
if execution.log { | ||
println!("dealloc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, maybe
println!("dealloc"); | |
println!("dealloc {:p}", ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/arc.rs
Outdated
@@ -60,6 +64,10 @@ impl Arc { | |||
self.obj.branch(Action::RefInc); | |||
|
|||
rt::execution(|execution| { | |||
if execution.log { | |||
println!("Arc::ref_inc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about including the ref count after increment in this message? We'd have to move the message down to where the ref count is incremented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/arc.rs
Outdated
@@ -90,6 +102,10 @@ impl Arc { | |||
self.obj.branch(Action::RefDec); | |||
|
|||
rt::execution(|execution| { | |||
if execution.log { | |||
println!("Arc::ref_dec"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, maybe this should include the ref count after decrement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/arc.rs
Outdated
@@ -45,6 +45,10 @@ pub(super) enum Action { | |||
impl Arc { | |||
pub(crate) fn new() -> Arc { | |||
rt::execution(|execution| { | |||
if execution.log { | |||
println!("Arc::new"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should include something identifying the Arc (maybe the internal loom object ID or a memory address?) in this message and the other messages in this module?
Although those don't convey a whole lot of semantic information about which Arc
in the program the messages refer to, they do allow matching the Arc::new
, Arc::ref_inc,
Arc::ref_decmessages and so on. Paired with logging in the code under test, users _could_ probably track different
Arc`s in their code based on an identifier added by loom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/atomic.rs
Outdated
@@ -128,6 +148,10 @@ pub(crate) fn fence(order: Ordering) { | |||
); | |||
|
|||
rt::synchronize(|execution| { | |||
if execution.log { | |||
println!("fence"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this include what the ordering of the fence was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rt/mod.rs
Outdated
@@ -43,6 +43,10 @@ where | |||
F: FnOnce() + 'static, | |||
{ | |||
execution(|execution| { | |||
if execution.log { | |||
println!("spawn"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to my other comments, I think it would be very helpful to include something identifying the thread to these messages. I think we can use the loom
internal object ID for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@hawkw Agreed on every point, looking into it. Thanks for the feedback! |
Used the following code for testing. It should trigger all trace points either directly or implicitly. Printing additional data in Not using Not using the function_name crate because it doesn't print the name of a struct Various log levels are supported (see the tracing docs), for example:
No messages are printed when the environment variable is not set, same as before. use loom;
use loom::cell::CausalCell;
use std::alloc::Layout;
use loom::alloc::{alloc, dealloc};
use loom::sync::Arc;
use loom::sync::atomic::{AtomicUsize, fence};
use std::sync::atomic::Ordering;
use loom::sync::Condvar;
use loom::sync::Mutex;
use loom::thread;
use std::cell::RefCell;
fn main() {
loom::model(|| {
let causal_cell = CausalCell::new(42);
unsafe { let _ = causal_cell.with_deferred(|x| *x + 1); }
unsafe { let _ = causal_cell.with_deferred_mut(|x| *x + 2); }
causal_cell.check();
causal_cell.check_mut();
println!("----------------------------------------------------------");
// XXX: `alloc` doesn't use `Allocation::new()`, so only
// `Allocation::drop` is printed. Tried using `new` in `alloc`, but
// it causes a panic for some reason.
let layout = Layout::new::<i32>();
unsafe {
let ptr = alloc(layout);
dealloc(ptr, layout);
}
println!("----------------------------------------------------------");
let mut arc = Arc::new(7);
let _ = arc.clone();
Arc::get_mut(&mut arc);
println!("----------------------------------------------------------");
let mut atomic = AtomicUsize::new(80);
let _ = atomic.load(Ordering::AcqRel);
let _ = atomic.store(99, Ordering::Release);
let _ = atomic.fetch_add(2, Ordering::SeqCst);
AtomicUsize::get_mut(&mut atomic);
fence(Ordering::Acquire);
println!("----------------------------------------------------------");
// From
// https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html
let pair = Arc::new((Mutex::new(false), Condvar::new()));
let pair2 = pair.clone();
thread::spawn(move || {
let (mutex, cvar) = &*pair2;
let mut started = mutex.lock().unwrap();
*started = true;
cvar.notify_one();
// cvar.notify_all();
});
let (mutex, cvar) = &*pair;
let mut started = mutex.lock().unwrap();
while !*started {
started = cvar.wait(started).unwrap();
}
println!("----------------------------------------------------------");
loom::thread::yield_now();
println!("----------------------------------------------------------");
let th1 = thread::spawn(|| {});
let th2 = thread::spawn(|| {});
th1.join().unwrap();
th2.join().unwrap();
println!("----------------------------------------------------------");
// From
// https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html
loom::thread_local!(static FOO: RefCell<u32> = RefCell::new(1));
FOO.with(|f| {
assert_eq!(*f.borrow(), 1);
*f.borrow_mut() = 2;
});
println!("----------------------------------------------------------");
});
} |
Let me know if you can think of a way to make this even more useful, or if you spot any issues! |
One issue I'm aware of is that some messages are now printed a bit later. Right after switching to tracing:
The most recent commit:
This happens because I moved some trace points a bit to be able to print more context. I don't think it matters a lot, it's just something to be aware of. Just a heads up. |
Another minor thing is that trace!(obj = ?self.obj, mutex = ?mutex, "Condvar::wait"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments on minor tracing
style nits, which aren't blockers for merging this PR. Also, I had some thoughts on potentially using tracing
's spans to include information about the iteration and thread in all the recorded events.
I think ideally, it would be fantastic if we had a span structure like this for all events loom
records:
test{name=my_loom_test}:execution{iter=15}:thread{id=1}: something happened...
(note that recording the test name may be difficult without asking the user to provide it, or providing a test macro...)
But, I think it would be fine to look into this in a follow-up PR or series of follow-ups; I think we should get some logging merged before we spend too much time on adding more detailed diagnostics.
println!(""); | ||
info!(""); | ||
info!(" ================== Iteration {} ==================", i); | ||
info!(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of logging a large header, what do you think about creating a separate tracing
span representing each iteration, with the iteration number as a field? That way, everything recorded during that iteration will be annotated with the iteration number.
This isn't necessarily a blocker for merging this PR, so it might be worth doing in a follow-up branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the header should be printed as is anyway at the info level (same for the thread one) because it's easier to read logs with it at the trace level. Also, at the info level this still provides
useful high-level information about the way threads/iterations are scheduled, without the overhead of trace messages.
As for using spans for iterations and threads, I'm all for it, but I think it should be done separately since (1) it requires some changes to pass spans around, (2) I don't know how much time I can spend on this PR, and (3) there's barely any logging in master now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for using spans for iterations and threads, I'm all for it, but I think it should be done separately since (1) it requires some changes to pass spans around, (2) I don't know how much time I can spend on this PR, and (3) there's barely any logging in master now.
I totally agree, this was intended as a suggestion for a future branch! Let's move forwards with this as-is and add spans afterwards.
Excellent suggestions, going to address them now! |
Okay, I think this is ready to be merged. Let me know if I missed something! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! Let's go ahead and merge it (unless @carllerche has anything to add). We can add more diagnostics in subsequent PRs.
Hello ! I'm using loom to check the validity of a library I'm developping, but I fail to see why a particular iteration trigger a bad state. This PR would help me A LOT, any chance that it will make it into master ? |
Considering the number of merge conflicts, it would probably take some work. |
I took the liberty to fix most conflicts here : fiahil@c3e667c Considering the differences between the two versions, a fresh review might be necessary ! |
You can open a new PR with your changes. |
Merged in #227 |
No description provided.