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

Use futures::executor::Spawn with a Notify impl. #15

Merged
merged 3 commits into from
Sep 18, 2018

Conversation

twittner
Copy link
Contributor

@twittner twittner commented Sep 6, 2018

Implement a custom Notify implementation which in turn notifies waiting tasks. This is similar to what rust-libp2p's mplex implementation does (see libp2p/rust-libp2p#455) for details.

@twittner twittner requested a review from tomaka September 6, 2018 13:28
Implement a custom `Notify` implementation which in turn notifies
waiting tasks. This is similar to what rust-libp2p's mplex
implementation does (see libp2p/rust-libp2p#455)
for details.
@tomaka
Copy link
Member

tomaka commented Sep 17, 2018

I'm not a fan of having a custom Hash implementation in the code.
I'd suggest just using the FnvHashMap instead (from the fnv crate).

Other than that, ideally there should be documentation in the notify module, most notably a note saying that insert_current() should be called from within a task context or it will panic.

@twittner
Copy link
Contributor Author

twittner commented Sep 17, 2018

I'm not a fan of having a custom Hash implementation in the code.
I'd suggest just using the FnvHashMap instead (from the fnv crate).

Hashing usize numbers is pointless and fnv still introduces unnecessary overhead (about 30%). I agree that having the custom hasher in here is not ideal. Should I maybe generalise it and publish a crate stateless-hasher or int-hasher to crates.io instead?


Some benchmark results:

cargo bench
    Finished release [optimized] target(s) in 0.01s                                                                                                                                                                  
     Running target/release/deps/bench-c3a9a26a0b5dbf0e

running 2 tests
test tests::bench_custom ... bench:       6,803 ns/iter (+/- 232)
test tests::bench_fnv    ... bench:       9,808 ns/iter (+/- 1,641)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out
#![feature(test)]

use std::{self, hash::{BuildHasherDefault, Hasher}, marker::PhantomData};

pub type BuildIntHasher<T> = BuildHasherDefault<Stateless<T>>;

pub type IntMap<K, V> = std::collections::HashMap<K, V, BuildIntHasher<K>>;

#[derive(Copy, Clone, Debug, Default)]
pub struct Stateless<T>(u64, PhantomData<T>);

impl Hasher for Stateless<usize> {
    fn finish(&self) -> u64 { self.0 }
    fn write(&mut self, _: &[u8]) { unimplemented!() }
    fn write_u8(&mut self, _: u8) {unimplemented!() }
    fn write_u16(&mut self, _: u16) { unimplemented!() }
    fn write_u32(&mut self, _: u32) { unimplemented!() }
    fn write_u64(&mut self, _: u64) { unimplemented!() }
    fn write_u128(&mut self, _: u128) { unimplemented!() }
    fn write_usize(&mut self, n: usize) { self.0 = n as u64 }
    fn write_i8(&mut self, _: i8) { unimplemented!() }
    fn write_i16(&mut self, _: i16) { unimplemented!() }
    fn write_i32(&mut self, _: i32) { unimplemented!() }
    fn write_i64(&mut self, _: i64) { unimplemented!() }
    fn write_i128(&mut self, _: i128) { unimplemented!() }
    fn write_isize(&mut self, _: isize) { unimplemented!() }
}

#[cfg(test)]
mod tests {
    extern crate test;
    use fnv::FnvBuildHasher;
    use self::test::Bencher;
    use std::collections::HashMap;

    const SIZE: usize = 1000;

    #[bench]
    fn bench_fnv(b: &mut Bencher) {
        let mut m = HashMap::with_capacity_and_hasher(SIZE, FnvBuildHasher::default());
        b.iter(|| {
            for i in 0_usize .. SIZE {
                m.insert(i, i);
            }
        })
    }

    #[bench]
    fn bench_custom(b: &mut Bencher) {
        let mut m = HashMap::with_capacity_and_hasher(SIZE, super::BuildIntHasher::default());
        b.iter(|| {
            for i in 0_usize .. SIZE {
                m.insert(i, i);
            }
        })
    }
}

@tomaka
Copy link
Member

tomaka commented Sep 17, 2018

I would take the 3 nanoseconds loss 😌

@twittner
Copy link
Contributor Author

@tomaka: This is ready for another review.

Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Let's fix the small nit and switch to the crates.io version of the hasher, and then merge.

src/notify.rs Outdated
///
/// # Panics
///
/// If called outside of a tokio task.
Copy link
Member

Choose a reason for hiding this comment

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

a futures task, not a tokio task.

@twittner twittner merged commit 5acf79e into libp2p:master Sep 18, 2018
@twittner twittner deleted the notifier branch September 18, 2018 09:36
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