fix(shwap/bitswap): Blockstore.GetSize: getting size with no compute #3894
+71
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wondertan
requested review from
renaynay,
vgonkivs,
walldiss and
cristaloleg
as code owners
Wondertan
force-pushed
the
shwap-bitswap-get-size
branch
3 times, most recently
from
2bef3ec to
ad5a7b5
Compare
|
If GetSize is used only for prioritisation of bitswap server hanling of requests, perhaps don't really need to calculate sizes. Alternative could be to return const values depending on type of the cid and our estimate of proper priority / load of handling. For examples:
This would allow us to remove the need for heavy reads to get the priority for serving of requests and will easy the pressure on cache. |
|
In sync, we agreed to keep the Size computation based on real blocksize to be safe. Making all sizes fixed to max block size may limit bitswap's throughput for small block sizes. |
renaynay
reviewed
Oct 31, 2024
|
It is covered |
walldiss
previously approved these changes
Oct 31, 2024
vgonkivs
previously approved these changes
Nov 1, 2024
|
Disabling the automerge for now. I want to do another change here |
Wondertan
force-pushed
the
shwap-bitswap-get-size
branch
from
f5c4f4c to
fc1e188
Compare
walldiss
previously approved these changes
Nov 14, 2024
Wondertan
force-pushed
the
shwap-bitswap-get-size
branch
from
fc1e188 to
8c953dc
Compare
Wondertan
force-pushed
the
shwap-bitswap-get-size
branch
from
e32030b to
8fc2000
Compare
renaynay
approved these changes
Nov 14, 2024
walldiss
approved these changes
Nov 15, 2024
Wondertan
added a commit
that referenced
this pull request
Nov 15, 2024
…3894) Bitswap prioritizes peers based on their active/pending work and the priority that peers set for requests(work) themselves. The prioritization happens on the `Get` operation of `Blockstore` not `GetSize`. However, we currently do the most expensive work in `GetSize` followed by `Get`. It is still part of the same WANT_BLOCK request(since #3813), and `GetSize` work is usually cached for `Get` to catch it. Still, the prioritization makes it so that the time between `GetSize` and `Get` can be pretty long, inducing cache misses and redoing the same work again, which is confirmed by profiles. Also, doing the expensive part in `GetSize` avoids Bitswap server's rate limiting. All this brings the need to make `GetSize` as lightweight as possible, leaving actual proof computed to `Get`. This PR achieves this by defining a constant max block size for every type block. --- 🎱 mb blonks certified
Wondertan
added a commit
that referenced
this pull request
Nov 15, 2024
…3894) Bitswap prioritizes peers based on their active/pending work and the priority that peers set for requests(work) themselves. The prioritization happens on the `Get` operation of `Blockstore` not `GetSize`. However, we currently do the most expensive work in `GetSize` followed by `Get`. It is still part of the same WANT_BLOCK request(since #3813), and `GetSize` work is usually cached for `Get` to catch it. Still, the prioritization makes it so that the time between `GetSize` and `Get` can be pretty long, inducing cache misses and redoing the same work again, which is confirmed by profiles. Also, doing the expensive part in `GetSize` avoids Bitswap server's rate limiting. All this brings the need to make `GetSize` as lightweight as possible, leaving actual proof computed to `Get`. This PR achieves this by defining a constant max block size for every type block. --- 🎱 mb blonks certified
Wondertan
added a commit
that referenced
this pull request
Nov 15, 2024
…3894) Bitswap prioritizes peers based on their active/pending work and the priority that peers set for requests(work) themselves. The prioritization happens on the `Get` operation of `Blockstore` not `GetSize`. However, we currently do the most expensive work in `GetSize` followed by `Get`. It is still part of the same WANT_BLOCK request(since #3813), and `GetSize` work is usually cached for `Get` to catch it. Still, the prioritization makes it so that the time between `GetSize` and `Get` can be pretty long, inducing cache misses and redoing the same work again, which is confirmed by profiles. Also, doing the expensive part in `GetSize` avoids Bitswap server's rate limiting. All this brings the need to make `GetSize` as lightweight as possible, leaving actual proof computed to `Get`. This PR achieves this by defining a constant max block size for every type block. --- 🎱 mb blonks certified
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Bitswap prioritizes peers based on their active/pending work and the priority that peers set for requests(work) themselves. The prioritization happens on the
Getoperation ofBlockstorenotGetSize. However, we currently do the most expensive work inGetSizefollowed byGet. It is still part of the same WANT_BLOCK request(since #3813), andGetSizework is usually cached forGetto catch it. Still, the prioritization makes it so that the time betweenGetSizeandGetcan be pretty long, inducing cache misses and redoing the same work again, which is confirmed by profiles. Also, doing the expensive part inGetSizeavoids Bitswap server's rate limiting.All this brings the need to make
GetSizeas lightweight as possible, leaving actual proof computed toGet. This PR achieves this by defining a constant max block size for every type block.🎱 mb blonks certified