Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(@libp2p/protocol-perf): Implement perf protocol #1604
feat(@libp2p/protocol-perf): Implement perf protocol #1604
Changes from 36 commits
c524755
0898dd0
2458fbc
8aee800
2ec297d
8ea1870
4de5595
9a7f9d8
65bfaf3
9bed517
c1b262f
fe79a72
d28dd92
9dd0248
6661424
79cbe19
118703a
7eb1a79
e21cda8
b1d89bd
ccd5f26
cf0981b
7f9c716
35c79a4
ec98ddf
8ba8db3
59dc3aa
f9b91cb
68dfd68
0bff620
94caca8
fb914fd
a28ff46
8b056fb
5e62a7b
3ad9ea3
0a4eae5
b2da056
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
From what I can see we're always going to be sending byte arrays made up of zeroes in this protocol?
If there's any data compression applied could this affect benchmark outputs?
Would sending random data be better?
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.
Both random bytes and zeros is fine by the specification. See past discussion by you, @achingbrain.
libp2p/specs#478 (comment)
All other implementations use zeroes.
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.
Zero is fine, random bytes can also be fine (as long as it's not a different set of random bytes, because then you may be limited by how fast you can generate random bytes).
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.
Look at me making the same point twice 😆
You can generate them ahead of time so this is solvable. I haven't measured it but in my head at least any network operation will be orders of magnitude slower than a pRNG?
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.
Let's measure it! On my machine (M1 Max):
That's 2.5 Gigabits/s, while fast is not fast enough to saturate a fairly common 10 gbps line.
(Yes you can fiddle with blocksizes, but I think 1500 is fair as it's a common MTU size.)
But what's the issue with sending zero bytes? Is it that you're afraid we'll silently introduce compression on a transport and thus not correctly measure throughput? If that's the case sending zero bytes is actually ideal because it would be obvious if there is compression happening.
There's also the question of arbitrarily compressing at a transport level. That kind of thing opens you up the TLS CRIME style attacks. So I don't think we should support compression without being application aware.