Fetch multiple pieces during object reconstruction #3158

teor2345 · 2024-10-22T01:23:57Z

This PR changes ObjectFetcher to use the multi-piece fetching methods of PieceProvider to get small numbers of pieces.

As part of this change, ObjectFetcher couldn't be used as a dyn trait any more, because of the generic bounds on get_pieces().

Code contributor checklist:

I have read, understood and followed contributing guide

crates/subspace-gateway/src/piece_getter.rs

shared/subspace-data-retrieval/src/piece_getter.rs

shared/subspace-data-retrieval/src/piece_fetcher.rs

shared/subspace-data-retrieval/src/segment_fetcher.rs

nazar-pc · 2024-10-22T12:54:47Z

shared/subspace-data-retrieval/src/segment_fetcher.rs

+    // - the number of remaining piece indexes gets smaller, eventually finishing the fetcher, or
+    // - the number of pending pieces gets smaller, eventually triggering another batch.
+    // We also exit early if we have enough pieces to reconstruct a segment.
+    'fetcher: while !piece_indexes.is_empty()


I would split this into two stages:

first try to download all 128 source pieces, if successful reconstruction will be very cheap and fast

as a fallback when actual reconstruction is needed, schedule more pieces to download, including parity

Right now it is implemented in a way that is a bit wasteful in terms of bandwidth (triggers more downloads than needed) and in terms of CPU usage (has overwhelmingly high chance of not getting 128 source pieces for cheap segment reconstruction).

This is already what the code does?

The first batch contains 128 source piece indexes. If all pieces in that batch succeed, then there aren’t any parity piece requests.

But as soon as any pieces fail, a batch of parity piece indexes is created, which contains exactly the number of pieces needed to compensate for those failures. Then all batches are polled concurrently.

The code assumes that any pieces that are still pending will succeed, so there’s no wasted downloads.

I see, the way it is written wasn't as clear, but now I understand what it does. If you exhaust the stream of pieces you've generated every time, why do you keep already finished streams in piece_streams (the question above)? I don't see how multiple streams can be pooled concurrently here.

I see, the way it is written wasn't as clear, but now I understand what it does.

Good feedback, I might split it into multiple methods so it's clearer.

flatten_unordered() polls all the streams in the vector concurrently, and can return the pieces from any stream.

ready_chunks() waits until at least one piece result is ready, then returns all the ready pieces as a vector. But if any pieces are still pending, they are left in the stream.

Then if any of the piece results in that vector are None, we add a batch of parity pieces to replace them.

We can definitely drop streams once they're done, I'll make some notes about how to do that.

I actually already wrote an efficient piece retrieval for reconstruction purposes in https://github.com/autonomys/subspace/blob/362c1f5dce076b6c13452977a533f9091d515bb1/crates/subspace-farmer-components/src/segment_reconstruction.rs

It might be a little simpler and it does try to download pieces in batches all the time, avoiding batches of 1 that you will most likely get majority of time after initial request due to the way ready_chunks works. It is overall less eager and tries to not do a lot of heavy lifting.

Can be extracted into a utility somewhere to return a segment worth of pieces and then reused in farmer code, here and in DSN sync on the node, where exactly the same logic will be necessary for piece retrieval, just what we do with those pieces is slightly different.

I think I'll do this in another PR

teor2345

I’ll turn some of my comments on this PR into code comments, so the structure of the code is clearer.

crates/subspace-gateway-rpc/src/lib.rs

shared/subspace-data-retrieval/src/piece_getter.rs

shared/subspace-data-retrieval/src/segment_fetcher.rs

teor2345 · 2024-10-22T20:24:03Z

shared/subspace-data-retrieval/src/segment_fetcher.rs

+    // - the number of remaining piece indexes gets smaller, eventually finishing the fetcher, or
+    // - the number of pending pieces gets smaller, eventually triggering another batch.
+    // We also exit early if we have enough pieces to reconstruct a segment.
+    'fetcher: while !piece_indexes.is_empty()


This is already what the code does?

The first batch contains 128 source piece indexes. If all pieces in that batch succeed, then there aren’t any parity piece requests.

But as soon as any pieces fail, a batch of parity piece indexes is created, which contains exactly the number of pieces needed to compensate for those failures. Then all batches are polled concurrently.

The code assumes that any pieces that are still pending will succeed, so there’s no wasted downloads.

shamil-gadelshin

Makes sense overall, just a question related to a special case.

shared/subspace-data-retrieval/src/segment_fetcher.rs

teor2345

Thank you both for the reviews! I'll work on a refactor to make this code clearer.

shared/subspace-data-retrieval/src/segment_fetcher.rs

teor2345 · 2024-10-23T23:29:41Z

shared/subspace-data-retrieval/src/segment_fetcher.rs

+    // - the number of remaining piece indexes gets smaller, eventually finishing the fetcher, or
+    // - the number of pending pieces gets smaller, eventually triggering another batch.
+    // We also exit early if we have enough pieces to reconstruct a segment.
+    'fetcher: while !piece_indexes.is_empty()


I see, the way it is written wasn't as clear, but now I understand what it does.

Good feedback, I might split it into multiple methods so it's clearer.

flatten_unordered() polls all the streams in the vector concurrently, and can return the pieces from any stream.

ready_chunks() waits until at least one piece result is ready, then returns all the ready pieces as a vector. But if any pieces are still pending, they are left in the stream.

Then if any of the piece results in that vector are None, we add a batch of parity pieces to replace them.

We can definitely drop streams once they're done, I'll make some notes about how to do that.

shared/subspace-data-retrieval/src/segment_fetcher.rs

teor2345

I've removed the segment changes, and I'll do that refactor in another PR. All other issues should be fixed now.

teor2345 · 2024-11-25T05:11:38Z

shared/subspace-data-retrieval/src/segment_fetcher.rs

+    // - the number of remaining piece indexes gets smaller, eventually finishing the fetcher, or
+    // - the number of pending pieces gets smaller, eventually triggering another batch.
+    // We also exit early if we have enough pieces to reconstruct a segment.
+    'fetcher: while !piece_indexes.is_empty()


I think I'll do this in another PR

Changes made (or deferred to another PR)

teor2345 · 2024-11-27T02:31:46Z

crates/subspace-gateway/src/piece_getter.rs

            .get_piece_from_archival_storage(piece_index, MAX_RANDOM_WALK_ROUNDS)
            .await)


@nazar-pc is this (and the similar code below) still needed after your PR #3259?

Yes, random walking is a cold storage fallback fr cases when a piece is not cached, in that case it is wandering around the network hoping to stumble upon someone storing a piece in their plot rather than cache

nazar-pc

Some nits, nothing critical

nazar-pc · 2024-11-27T04:36:50Z

crates/subspace-gateway/src/commands/run.rs

@@ -102,7 +102,7 @@ pub async fn run(run_options: RunOptions) -> anyhow::Result<()> {
        Semaphore::new(out_connections as usize * PIECE_PROVIDER_MULTIPLIER),
    );
    let piece_getter = DsnPieceGetter::new(piece_provider);
-    let object_fetcher = ObjectFetcher::new(piece_getter, erasure_coding, Some(max_size));
+    let object_fetcher = ObjectFetcher::new(piece_getter.into(), erasure_coding, Some(max_size));


This puzzled me for a moment, this is the first time I saw From used instead of Arc::new() 🤔

nazar-pc · 2024-11-27T04:42:18Z

crates/subspace-gateway/src/piece_getter.rs

            .get_piece_from_archival_storage(piece_index, MAX_RANDOM_WALK_ROUNDS)
            .await)


Yes, random walking is a cold storage fallback fr cases when a piece is not cached, in that case it is wandering around the network hoping to stumble upon someone storing a piece in their plot rather than cache

nazar-pc · 2024-11-27T04:47:47Z

shared/subspace-data-retrieval/src/piece_fetcher.rs

+    let mut pieces = Vec::new();
+    pieces.resize(piece_indexes.len(), Piece::default());


This will work, but it is a bit unfortunate that we'll be allocating so much upfront (each piece) and then simply throwing those allocations away.

I'd probably push all results into HashMap<PieceIndex, Piece> and then extract them at the end by piece index (pieces have copy-on-write behavior, so it'll be efficient, we don't need to remove individual pieces, can just drop the whole hashmap at the end at once).

teor2345 added 3 commits October 22, 2024 09:48

Make ObjectFetcher generic rather than dyn trait, use anyhow

74a30fe

Add ObjectPieceGetter::get_pieces()

01750d2

Fetch multiple pieces in the piece fetcher

170588b

teor2345 added the enhancement New feature or request label Oct 22, 2024

teor2345 self-assigned this Oct 22, 2024

teor2345 requested review from nazar-pc and rg3l3dr as code owners October 22, 2024 01:23

nazar-pc previously requested changes Oct 22, 2024

View reviewed changes

Base automatically changed from gateway-args to main October 22, 2024 12:57

teor2345 commented Oct 22, 2024

View reviewed changes

teor2345 enabled auto-merge October 22, 2024 20:25

shamil-gadelshin reviewed Oct 23, 2024

View reviewed changes

shared/subspace-data-retrieval/src/segment_fetcher.rs Outdated Show resolved Hide resolved

teor2345 commented Oct 23, 2024

View reviewed changes

teor2345 mentioned this pull request Nov 19, 2024

Implement DSN Data Retrieval (protocol side) #3106

Open

44 tasks

teor2345 marked this pull request as draft November 21, 2024 07:23

auto-merge was automatically disabled November 21, 2024 07:23
Pull request was converted to draft

teor2345 added 4 commits November 25, 2024 14:53

Merge branch 'main' into obj-piece-list

ab23ed3

Fall back to archival storage when fetching pieces

d163158

Fix bug in download_pieces()

d12935d

Remove an unnecessary TODO

baff7b7

teor2345 force-pushed the obj-piece-list branch from f488e3a to baff7b7 Compare November 25, 2024 05:12

teor2345 commented Nov 25, 2024

View reviewed changes

teor2345 requested review from shamil-gadelshin and nazar-pc November 25, 2024 05:13

teor2345 marked this pull request as ready for review November 25, 2024 05:13

teor2345 enabled auto-merge November 25, 2024 05:13

Merge branch 'main' into obj-piece-list

b9150af

teor2345 force-pushed the obj-piece-list branch from 81101f9 to b9150af Compare November 27, 2024 01:13

Fix argument types after merge

0755013

teor2345 commented Nov 27, 2024

View reviewed changes

nazar-pc approved these changes Nov 27, 2024

View reviewed changes

teor2345 added this pull request to the merge queue Nov 27, 2024

Merged via the queue into main with commit 239d791 Nov 27, 2024
8 checks passed

teor2345 deleted the obj-piece-list branch November 27, 2024 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch multiple pieces during object reconstruction #3158

Fetch multiple pieces during object reconstruction #3158

teor2345 commented Oct 22, 2024 •

edited

Loading

nazar-pc Oct 22, 2024

teor2345 Oct 22, 2024

nazar-pc Oct 22, 2024

teor2345 Oct 23, 2024

nazar-pc Oct 24, 2024

teor2345 Nov 25, 2024

teor2345 left a comment

teor2345 Oct 22, 2024

shamil-gadelshin left a comment

teor2345 left a comment

teor2345 Oct 23, 2024

teor2345 left a comment

teor2345 Nov 25, 2024

teor2345 Nov 27, 2024

nazar-pc Nov 27, 2024

nazar-pc left a comment

nazar-pc Nov 27, 2024

nazar-pc Nov 27, 2024

nazar-pc Nov 27, 2024

		.get_piece_from_archival_storage(piece_index, MAX_RANDOM_WALK_ROUNDS)
		.await)

		let mut pieces = Vec::new();
		pieces.resize(piece_indexes.len(), Piece::default());

Fetch multiple pieces during object reconstruction #3158

Fetch multiple pieces during object reconstruction #3158

Conversation

teor2345 commented Oct 22, 2024 • edited Loading

Code contributor checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamil-gadelshin left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Oct 22, 2024 •

edited

Loading