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

support bitswap configurability #8268

Merged
merged 27 commits into from
Aug 18, 2021
Merged

support bitswap configurability #8268

merged 27 commits into from
Aug 18, 2021

Conversation

aschmahmann
Copy link
Contributor

Part of #8233

@petar petar marked this pull request as ready for review July 26, 2021 20:06
core/node/core.go Outdated Show resolved Hide resolved
core/node/groups.go Show resolved Hide resolved
core/commands/bitswap.go Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
Comment on lines +872 to +892
This number controls fairness and can very from 250Kb (very fair) to 10Mb (less fair, with more work
dedicated to peers who ask for more). Values below 250Kb could cause thrashing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Were these experimental numbers you came up with?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. They are more of an example to demonstrate the interaction between fairness and thrashing. The 250KB is derived theoretically using the observation that 65KB is one packet, so utilizing anything under 5 packets per send (and then switching to another peer) feels like it may cause thrashing. In the extreme 65Kb (1 packet per send) would likely be thrashing because the block preparation pipeline (which involves a context switch to disk IO) would start costing more than the send itself.

docs/config.md Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Some nits around config.md

docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
lidel added a commit that referenced this pull request Aug 17, 2021
also, move toc to the top, as noted in
#8268 (comment)
@lidel
Copy link
Member

lidel commented Aug 17, 2021

Applied suggested changes to /docs/config.md and added docs for optionalInteger type.

lidel and others added 3 commits August 17, 2021 21:12
also, move toc to the top, as noted in
#8268 (comment)
- extracts bitswap init to separate file and defines implicit defaults as consts
- fixes a typo where BitswapTaskWorkerCount was 128 (instead of 8)
  and BitswapEngineBlockstoreWorkerCount was 8 (instead of 128)
@lidel
Copy link
Member

lidel commented Aug 17, 2021

  • refactor: moved bitswap init and consts with defaults to core/node/bitswap.go (refactor: core/node/bitswap.go #8349)
  • rebased this PR on top of master with latest ipld prime (easier to resolve conflicts this way)

CI for go-ipfs-http-client fails but @aschmahmann is looking into it

@aschmahmann aschmahmann merged commit 7448340 into master Aug 18, 2021
@aschmahmann aschmahmann deleted the feat/bitswap-config branch August 18, 2021 18:15
This was referenced Aug 19, 2021
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.

4 participants