-
Notifications
You must be signed in to change notification settings - Fork 993
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
[mplex] Small enhancements. #1785
Conversation
Also replace fnv hashing with nohash-hasher.
muxers/mplex/src/io.rs
Outdated
/// Randomly generated and mainly intended to improve log output | ||
/// by scoping substream IDs to a connection. | ||
#[derive(Clone, Copy)] | ||
struct Id(u32); |
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.
If I am not mistaken with the birthday paradox in mind on a server with ~100_000 connections the chance for a collision is > 50%. While these identifiers are only used for logging (for now) is the expected performance win with a u32 instead of a u64 worth the risk of a collision?
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.
It's not a matter of performance but of compact representation in the logs. Compare:
[2020-10-14T11:01:44Z DEBUG libp2p_mplex::io] f00c8822: New inbound substream: (18/receiver) (total 19)
[2020-10-14T11:01:44Z TRACE libp2p_mplex::io] f00c8822: Received Open { stream_id: RemoteStreamId { num: 19, role: Dialer } }
with
[2020-10-14T11:02:40Z TRACE libp2p_mplex::io] 19350c447f51a00a: Received Data { stream_id: RemoteStreamId { num: 0, role: Dialer }, data: b"Hello world" }
[2020-10-14T11:02:40Z TRACE libp2p_mplex::io] 19350c447f51a00a: Buffering b"Hello world" for stream (0/receiver) (total: 139)
I don't mind using a u64
instead if you don't think the long representation in the logs is too distracting. We could also use a u64
but cast to u32
for logging, but that may just add confusion.
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.
What do you think of f6bb354?
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 am not in favor of introducing a global variable. While still only used for logging, I would find it confusing that connection ids are monotonic e.g. when executing tests in serial, and possibly not monotonic (per test) when run concurrently.
Even though it is a bit verbose, I would prefer the u64, simply as there are no edge-cases. But I am fine with the u32 as well (maybe with a comment pointing out the probability of collisions).
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 am not in favor of introducing a global variable. While still only used for logging, I would find it confusing that connection ids are monotonic e.g. when executing tests in serial, and possibly not monotonic (per test) when run concurrently.
I don't quite understand your concerns with monotonic connection IDs or why you are referring to a module-scoped atomic counter as a global variable, especially in contrast to the use of a global random number generator, but I'm OK with the random u64
for 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.
See b98234b.
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.
Looks good to me.
This PR builds on #1784 (proper diff here), making a few further small improvements:
fnv
withnohash-hasher
to avoid unnecessary hashing of substream IDs.