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

372: Implement Eq and Hash for socket2::SockAddr #374

Merged
merged 5 commits into from
Jan 8, 2023

Conversation

lshlyapnikov
Copy link
Contributor

@lshlyapnikov lshlyapnikov commented Jan 2, 2023

Closes: #372.

This PR started as: #373, which was based off of v0.4.x by mistake, changing the base to master.

Cannot derive Eq and Hash for socket2::SockAddr because sockaddr_storage does not have both traits derived/implemented. sockaddr_storage is defined within s_no_extra_traits, my assumption it is expected to stay this way and I should not derive/implement Eq and Hash for sockaddr_storage.

TODO:

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/sockaddr.rs Show resolved Hide resolved
@lshlyapnikov
Copy link
Contributor Author

lshlyapnikov commented Jan 8, 2023

Benchmarks socket2::SockAddr::hash vs socket2::SockAddr::as_socket + std::net::SocketAddr::hash

https://github.com/lshlyapnikov/socket2/blob/sockaddr-quickcheck/benches/sockaddr_bench.rs

    bench.iter(|| {
        x.hash(&mut hasher);
        hasher.finish()
    })
    bench.iter(|| {
        x.as_socket().map(|y| y.hash(&mut hasher));
        hasher.finish()
    })
$ cargo bench
...
Running benches/sockaddr_bench.rs (target/release/deps/sockaddr_bench-8434022fa4125c76)

running 2 tests
test benchmark_as_socket_hash ... bench:          42 ns/iter (+/- 21)
test benchmark_hash           ... bench:          19 ns/iter (+/- 8)

I will keep these benchmarks in my fork. Let me know when/if you want to add them to the master.

@lshlyapnikov lshlyapnikov marked this pull request as ready for review January 8, 2023 17:40
@Thomasdezeeuw Thomasdezeeuw merged commit 35ee2ff into rust-lang:master Jan 8, 2023
@Thomasdezeeuw
Copy link
Collaborator

Running benches/sockaddr_bench.rs (target/release/deps/sockaddr_bench-8434022fa4125c76)

running 2 tests
test benchmark_as_socket_hash ... bench: 42 ns/iter (+/- 21)
test benchmark_hash ... bench: 19 ns/iter (+/- 8)

Looks pretty decent.

I will keep these benchmarks in my fork. Let me know when/if you want to add them to the master.

I think that's ok for now. If we ever get a complaint about the performance we can add them.

Thanks for the pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants