Parallel bitswap retrieval #149
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 68.45% 69.64% +1.18%
==========================================
Files 60 62 +2
Lines 4521 5040 +519
==========================================
+ Hits 3095 3510 +415
- Misses 1268 1337 +69
- Partials 158 193 +35
|
rvagg
force-pushed
the
rvagg/preloader
branch
9 times, most recently
from
42fbe78 to
d92b3bf
Compare
|
Marked as ready for review but do not merge at this stage due to this relying on a branch in go-ipld-prime that's not merged and still rough. That's my next task. |
rvagg
force-pushed
the
rvagg/preloader
branch
3 times, most recently
from
f50dd1b to
24b1cbd
Compare
There was a problem hiding this comment.
A few comments, nothing major -- when IPLD fix is done I'm mostly ready to merge this
I agree, tracking all the various stores is a bit unweildy. It might be nice to put a doc.go in pkg/storage with a package comment explaining what the various recorders are and why you would use one or the other.
pkg/retriever/bitswapretriever.go
Outdated
| if tmpDir == "" { | ||
| tmpDir = os.TempDir() | ||
| } | ||
| cacheStore := storage.NewDeferredCarStorage(tmpDir) |
There was a problem hiding this comment.
could the preload cache be fully in memory? presumably we don't need that much of a preload per active retrieval and by keeping those working sets in memory we save additional disk latencies
There was a problem hiding this comment.
Yes .. so this started off as in-memory (it's based on @hannahhoward's fully in-memory buffering preload store in the go-ipld-prime PR) but the concern is that there are real cases where a preloader will fill up memory, especially in the Saturn case where you're doing many parallel fetches. e.g. plain unixfs shard with 256 links and you only care about 1 initially, so you go down the next, preloader works on the other 255, the next jump has 256 more, etc. onward down until you come back up and start chewing away at the next branch; that could compound quickly with many parallel fetches.
Future work for this general preloading approach I imagined would have both an in-memory and a filesystem backed storage, where your in-memory objects can overflow into the filesystem if you end up holding too much. The vast majority of cases will not need more than a couple of blocks from the preloader so this would speed things up significantly.
But there's another issue that gets in the way - the traverser needs access to blocks its already passed for those DAG cases where you re-encounter blocks via different paths. They're less common with UnixFS but can happen, and we're also not limited to UnixFS here either. SO that means we need to hang onto the full suite of blocks for the life of a retrieval anyway. Using the approach I've gone with here we just put them all into the same basket on disk, using a CAR as a temporary store, not caring about the ordering, so the blocks are available to the preloader or the loader and in the case of a re-load via the loader. But it's also true that this could be done in memory up to some limit! So I'd really like to work on a storage system that starts in memory but overflows to disk, which we can then use for all the cases we need it in Lassie - graphsync, bitswap, preloader. The user only ever gets a nicely ordered CARv1 and we get to mess around with different ways of juggling blocks internally.
There was a problem hiding this comment.
For the selector we're using here do we actually need block data on re-encounter, or would it be sufficient to keep just the list of encountered CIDs so that we can properly answer 'Has' to remember which ones have already been interacted with?
There was a problem hiding this comment.
For the ones we're using I think we probably could .. we do allow for arbitrary selectors in the retrieval Request that can be passed in when using Lassie as a library, and this is yet again one of those cases where it'd be nice to be able to differentiate between selectors that care about revisits and those that don't.
For the main cases we're focused on for now, for the purposes of Saturn (these simple path-based selectors), we could probably get away with just a SkipMe{} whenever the LinkSystem is asked for a block that it's already encountered. In which case, just maintaining a list of CIDs would work. @hannahhoward is there anything in the graphsync traverser that would complicate this kind of approach (if we were hypothetically able to restrict the types of selectors we're willing to work with)?
I think maybe not in scope for this batch of work, but we could explore options for the "have simple selector, won't store anything but a list of CIDs we've already spat out" case later.
There was a problem hiding this comment.
OK so the names are good enough that I think I get what all of them do now.
So:
- DeferredCarWriter is basically the thing that allows you do defer writing the header or anything else till first block write. And then OnPut is used to do that dance in the IPFS handler to send all the headers when the first block is ready to go through. It deals with write only CARv1.
- DeferredCarStorage is the temp read/write store, but again, we defer making the file till first block write
- CachingTempStore is the big coordinator, mixing a DeferredCarStorage & and DeferredCarWriter, but then also exposing the two kinds of storage up to the Request logic -- the regular write for real store and the preload store.
Anyway, I think with this I'm ready to merge. Remaining comments are non-blocking. Merge whenever you want.
WIP, but it works at least. Depends on getting ipld/go-ipld-prime#452 right and landed and there's lots of additional work to do here to make it nicely integrated and configurable.
For now there's some debugging stats coming out just to prove that it's working properly (better proof to come later). At the end of
TestHttpFetch/bitswap_large_sharded_directorywe get:i.e. only one complete miss - the first block, past that 87 requests were for in-progress block fetches and 178 were for blocks we've already fetched.