Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

availability-recovery: move cpu burners in blocking tasks #7417

Merged
merged 10 commits into from
Jul 4, 2023

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Jun 22, 2023

Fixes #7411

TODO:

  • Address some availability data cloning issues in the impl
  • Ensure chunks and backing group recovery works perfectly as before
  • Versi burn-in analysis

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 22, 2023
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

burning this in on Versi as part of #7378

@sandreim
Copy link
Contributor Author

sandreim commented Jun 22, 2023

Versi burn-in shows an impressive CPU utilization of the reconstructed_data_matches_root (re-encode in the screenshot) with 2.5MB PoVs.

Screenshot 2023-06-22 at 16 38 04

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim marked this pull request as ready for review June 23, 2023 08:36
@sandreim sandreim requested review from eskimor and ordian June 23, 2023 08:36
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
node/subsystem-types/src/errors.rs Outdated Show resolved Hide resolved
@sandreim sandreim added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 23, 2023
node/subsystem-types/src/errors.rs Outdated Show resolved Hide resolved
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
@burdges
Copy link
Contributor

burdges commented Jun 23, 2023

How long do these tasks take to finish?

It'd likely be better for cache locality if https://github.com/paritytech/reed-solomon-novelpoly itself were made multi-threaded, so then its tables could spend less time in L1. It's possible altering the tower of extension fields changes this somewhat, so maybe not the first thing to try, but otoh maybe easy, like just a rayon loop here

Update: L1 is per core, so this is not really true.

@sandreim
Copy link
Contributor Author

sandreim commented Jul 3, 2023

How long do these tasks take to finish?

It'd likely be better for cache locality if https://github.com/paritytech/reed-solomon-novelpoly itself were made multi-threaded, so then its tables could spend less time in L1. It's possible altering the tower of extension fields changes this somewhat, so maybe not the first thing to try, but otoh maybe easy, like just a rayon loop here

Yes, pretty sure we can be faster here, but I expect that in practice the PoVs would rarely be this big (vs current synthetic Glutton test). This change set just makes sure we don't block other async tasks when reconstructing or re-encoding after data has been retrieved.

Screenshot 2023-07-03 at 11 07 12

@burdges
Copy link
Contributor

burdges commented Jul 3, 2023

Reencode takes almost 50% longer than reconstruct. I guess reconstruct does some extra work, as reencode should produce 4x as much data.

We should ask soramitsu to look at the rust code, given their leopard fork claims to do this 2x faster.
https://github.com/soramitsu/erasure-coding-crust

Your benchmarks are for 2.5 megs yes? We're actually 4x faster than their claims, but I'm not really sure what their test setup is, like what they mean by VM.

@sandreim
Copy link
Contributor Author

sandreim commented Jul 3, 2023

Reencode takes almost 50% longer than reconstruct. I guess reconstruct does some extra work, as reencode should produce 4x as much data.

We should ask soramitsu to look at the rust code, given their leopard fork claims to do this 2x faster. https://github.com/soramitsu/erasure-coding-crust

Your benchmarks are for 2.5 megs yes? We're actually 4x faster than their claims, but I'm not really sure what their test setup is, like what they mean by VM.

Yes, it is 2.5MiB, but we would need to be sure we have similar test env and input to claim that either is faster.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim requested a review from ordian July 3, 2023 13:48
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Some thoughts:

  1. How quick is the erasure encoding/decoding for very small PoVs ... could it be that the overhead of sending the data to a different thread is not worth it for those?
  2. The architecture of sending to a different thread/task, while we don't have anything else to do is a bit counter intuitive at first - worth adding comments, describing what you are doing here.

node/network/availability-recovery/src/lib.rs Outdated Show resolved Hide resolved
@sandreim
Copy link
Contributor Author

sandreim commented Jul 3, 2023

Looks good overall.

Some thoughts:

  1. How quick is the erasure encoding/decoding for very small PoVs ... could it be that the overhead of sending the data to a different thread is not worth it for those?

Min value seems to be 96ms on some node (representative of 32K PoVs), max value is for 2.5MiB PoVs.

Screenshot 2023-07-03 at 23 24 13
  1. The architecture of sending to a different thread/task, while we don't have anything else to do is a bit counter intuitive at first - worth adding comments, describing what you are doing here.

Sure, will add some comments

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

sandreim commented Jul 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7e1e635 into master Jul 4, 2023
3 checks passed
@paritytech-processbot paritytech-processbot bot deleted the sandreim/av_recovery_thread branch July 4, 2023 09:50
@burdges
Copy link
Contributor

burdges commented Jul 4, 2023

Min value seems to be 96ms on some node (representative of 32K PoVs), max value is for 2.5MiB PoVs.

Is this reconstruction only? It's maybe the cost of building the walsh table. We tried to embed all the tables as constants, although initially we generated them at process start up, but you always need some table specific to the chunks form which your reconstructing.

Interestingly, this table could be reused if you've exactly the same chunks. I doubt this helps us, but maybe it buys you some orthogonality in paritytech/polkadot-sdk#607, although probably no.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

availability-recovery: run CPU intensive work on separate thread
6 participants