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

Potential subtle unsafe memory reclaim #237

Closed
wants to merge 17 commits into from
Closed

Potential subtle unsafe memory reclaim #237

wants to merge 17 commits into from

Conversation

Pslydhh
Copy link
Contributor

@Pslydhh Pslydhh commented Nov 27, 2018

When we guard.defer/defer_destroy(ptr), we must ensure ptr can't be acquire after this moment.
But receivers maybe install the block first, update the 'head' and destroy the old one second, then senders start using old objects(install the next block, but old one has been reclaimed).

Happen to when head_index +1 == tail_index(means avoiding the empty channel), and receivers execute the codes and complete it more quickly:

    if offset + 1 == BLOCK_CAP {
        install_next_block();
        unsafe {
            guard.defer_destroy(head_ptr);
        }
    }

And now receivers can't acquire the old block, but senders maybe still reference it.
So I improve it through update the 'tail'(If not yet) in 'install_next_block', before defer it.
This is a question that I reasoned about, I didn't try to show it.

@ghost ghost mentioned this pull request Nov 27, 2018
@Pslydhh Pslydhh closed this Nov 28, 2018
@Pslydhh
Copy link
Contributor Author

Pslydhh commented Nov 28, 2018

queue_base_block is an experimental queue looks like the queue in crossbeam::channel::unbounded, it maybe better than Michael-Scott lock-free queue in crossbeam::epoch to speed up GC.
But Its perfermance seems very strange Under some scenes. I would like to explain later.

@Pslydhh Pslydhh reopened this Nov 28, 2018
@ghost
Copy link

ghost commented Nov 28, 2018

Thanks again for discovering the bug, that is a good catch! :)

But Its perfermance seems very strange Under some scenes. I would like to explain later.

Are you thinking about the case where mem::size_of::<T>() is large?

@Pslydhh
Copy link
Contributor Author

Pslydhh commented Nov 28, 2018

Thanks again for discovering the bug, that is a good catch! :)

But Its perfermance seems very strange Under some scenes. I would like to explain later.

Are you thinking about the case where mem::size_of::<T>() is large?

Maybe, you means threads blocked by OS's alloc/free operation?
my scenario is:

extern crate crossbeam;
use std::thread;

fn main() {
    let q = crossbeam::queue::MsQueue::new();
    for j in 0..100 {
        println!("time: {:?}", j);
        let now = std::time::Instant::now();
        let _ = crossbeam::scope(|s| {
            for _ in 0..4 {
                s.spawn(|_| { 
                    for _ in 0..25000000 {
                        let _ = q.push(1); 
                    }
                });
            }
        });
        let elapsed = now.elapsed();
        println!("send duration: {} secs {} nanosecs\n",elapsed.as_secs(), elapsed.subsec_nanos());
        
        let now = std::time::Instant::now();
        let _ = crossbeam::scope(|s| {
            for _ in 0..4 {
                s.spawn(|_| {
                    for _ in 0..25000000 {
                         assert_eq!(q.try_pop().unwrap(), 1);
                    }
                });
            }
        });
        let elapsed = now.elapsed();
        println!("recv duration: {} secs {} nanosecs\n", elapsed.as_secs(), elapsed.subsec_nanos());
        thread::sleep(std::time::Duration::from_millis(2000));
    }
}

The number is huge. I want to test the stability of this memory-reclaimed.
But as time go, recv duration is bigger and bigger...

time: 0
send duration: 15 secs 489399457 nanosecs
recv duration: 12 secs 416294755 nanosecs
time: 1
send duration: 15 secs 485358920 nanosecs
recv duration: 16 secs 37373045 nanosecs
time: 2
send duration: 15 secs 374774506 nanosecs
recv duration: 18 secs 13075211 nanosecs
time: 3
send duration: 15 secs 471223868 nanosecs
recv duration: 24 secs 858503132 nanosecs
time: 4
send duration: 15 secs 302331564 nanosecs
recv duration: 31 secs 65750669 nanosecs
time: 5
send duration: 15 secs 327771159 nanosecs
recv duration: 38 secs 381148911 nanosecs
time: 6
send duration: 15 secs 176869270 nanosecs
recv duration: 50 secs 510533982 nanosecs
.......
OS: windows 10, 8G, 4cores Intel cores

As time go, at the recv duration stage this process(5 threads) 's CPU utilization ratio is getting smaller and smaller, its' memory is even no reduction at a huge capacity(such as 3G), It seems like some threads no longer run, but there is no block or sleep operation in codes. In my test the original MSQueue is not the case, and the algorithm is stable. It's confusing to me, Maybe I missing something?

Actually, I has another more complex approach, but It is also suffer from above question.
In addition, try_pop should be add? It seems that it can't be compiled with or without it.

@Pslydhh Pslydhh closed this Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant