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

docs: include required chunk size in perf readme #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achingbrain
Copy link
Member

I almost changed the default to something bigger because it improves the benchmark results..

It's probably worth stating explicitly what the chunk size is so every implementation uses the same value for consistency.

I almost changed the default to something bigger because it improves the benchmark results..

It's probably worth stating explicitly what the chunk size is so every implementation uses the same value for consistency.
@achingbrain achingbrain requested a review from mxinden November 7, 2023 10:22
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

In what way do you expect the high level application chunk size to matter?

We do chunking at the TCP level, Noise level and Yamux level. We do chunking at the QUIC level. Once the application (here the perf protocol) provides a continuous flow of data, i.e. fill the pipe, why does it matter whether that is happening in 64KiB or 128KiB chunks?

@mxinden
Copy link
Member

mxinden commented Nov 8, 2023

because it improves the benchmark results..

Great. Then I suggest increasing it in js-libp2p. That said, worth investigating why js-libp2p is CPU bottlenecked on the chunk size?

@achingbrain
Copy link
Member Author

Then I suggest increasing it in js-libp2p

The point of this PR is so that all implementations use the same chunk size so there is a better basis for comparison. Is this not something we want?

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