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.

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

Add miri CI test #37

Merged
merged 3 commits into from
Jan 30, 2020
Merged

Add miri CI test #37

merged 3 commits into from
Jan 30, 2020

Conversation

jonhoo
Copy link
Owner

@jonhoo jonhoo commented Jan 27, 2020

Currently blocked on rust-lang/miri#1150 landing on nightly and on crossbeam-rs/crossbeam#458.

@jonhoo jonhoo mentioned this pull request Jan 27, 2020
@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #37 into master will decrease coverage by 1.23%.
The diff coverage is 60%.

Impacted Files Coverage Δ
src/map.rs 88.3% <60%> (-1.77%) ⬇️

@jonhoo
Copy link
Owner Author

jonhoo commented Jan 29, 2020

@danielSanchezQ Interesting -- it looks like the tests from #36 found a memory leak. Want to take a stab at fixing them? The logs are here, and the relevant entries are:

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #4 0x5609cbc139e2 in crossbeam_epoch::atomic::Owned$LT$T$GT$::new::hedac2cb2a6626c3b /home/vsts/.cargo/git/checkouts/crossbeam-308b9896d3fb1a0a/483d347/crossbeam-epoch/src/atomic.rs:664:19
    #5 0x5609cbc00671 in flurry::map::HashMap$LT$K$C$V$C$S$GT$::put::h8a55c48053512f17 /home/vsts/work/1/s/src/map.rs:362:19
    #6 0x5609cbbfff04 in flurry::map::HashMap$LT$K$C$V$C$S$GT$::insert::hf7a9b82571be5615 /home/vsts/work/1/s/src/map.rs:346:8
    #7 0x5609cbc08bb9 in flurry::map::replace_existing_observed_value_matching::_$u7b$$u7b$closure$u7d$$u7d$::h79bf84ff713dd6f5 /home/vsts/work/1/s/src/map.rs:1710

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #4 0x5609cbc139e2 in crossbeam_epoch::atomic::Owned$LT$T$GT$::new::hedac2cb2a6626c3b /home/vsts/.cargo/git/checkouts/crossbeam-308b9896d3fb1a0a/483d347/crossbeam-epoch/src/atomic.rs:664:19
    #5 0x5609cbc00671 in flurry::map::HashMap$LT$K$C$V$C$S$GT$::put::h8a55c48053512f17 /home/vsts/work/1/s/src/map.rs:362:19
    #6 0x5609cbbfff04 in flurry::map::HashMap$LT$K$C$V$C$S$GT$::insert::hf7a9b82571be5615 /home/vsts/work/1/s/src/map.rs:346:8
    #7 0x5609cbc08b99 in flurry::map::replace_existing::_$u7b$$u7b$closure$u7d$$u7d$::h9f40593e19e439cb /home/vsts/work/1/s/src/map.rs:1698

Specifically, it looks like the value inserted here for replace_existing

flurry/src/map.rs

Line 1695 in 9544337

map.insert(42, 42, &guard);

and here for replace_existing_observed_value_matching

flurry/src/map.rs

Line 1707 in 9544337

map.insert(42, 42, &guard);

are not freed when we later replace them. I'm not entirely sure why, but some prints may help clarify the situation?

@danielSanchezQ
Copy link
Contributor

Sure, I’ll investigate the issue!

@danielSanchezQ
Copy link
Contributor

danielSanchezQ commented Jan 30, 2020

This piece of code solves the memory leak:

if let Some(nv) = new_value {
    let dropable_old = n.value.load(Ordering::SeqCst, guard);
    n.value.store(Owned::new(nv), Ordering::SeqCst);
    // we are just replacing entry value and we do not want to remove the node
    // so we stop iterating here

    // force droping the old value
    drop(unsafe {dropable_old.into_owned()});
    break;
}

I'm forcing the drop. Probably is not the best way, but it gives some hints.
I am not sure why the resource is not being dropped, where could we have another shared pointer to that?

@jonhoo
Copy link
Owner Author

jonhoo commented Jan 30, 2020

Ah, no, I see what's going on! The break inside that if means that

old_value = Some(ev);

does not get executed, which in turn means that the deferred destroy further down does not get executed

flurry/src/map.rs

Line 1238 in 9544337

unsafe { guard.defer_destroy(val) };

I think this would be fixed by moving

flurry/src/map.rs

Line 1187 in 9544337

old_val = Some(ev);

up to just above the if. Will implement that!

@jonhoo
Copy link
Owner Author

jonhoo commented Jan 30, 2020

In fact, thinking some more on it, I think this assertion is wrong:

flurry/src/map.rs

Line 1697 in 9544337

assert!(old.is_none());

It should be Some since we replaced an old value! The fix above also fixes that.

@jonhoo jonhoo merged commit c4e5d58 into master Jan 30, 2020
@jonhoo
Copy link
Owner Author

jonhoo commented Jan 30, 2020

That worked 🎉

@jonhoo jonhoo deleted the miri-ci branch January 30, 2020 13:34
@danielSanchezQ
Copy link
Contributor

Aaaah, I see it now. Actually it was my fault there. I understood that since we are not really removing the entry nothing should be returned. But ofc it makes sense that the old value is returned. Sorry for that!

jonhoo referenced this pull request Mar 17, 2020
These are probably spurious; see
crossbeam-rs/crossbeam#464
and
spacejam/sled#937 (comment)

And besides, we're now running both the leak sanitizer and the address
sanitizer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants