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

Shred entries in parallel #6180

Merged
merged 4 commits into from Oct 8, 2019
Merged

Shred entries in parallel #6180

merged 4 commits into from Oct 8, 2019

Conversation

@carllin
Copy link
Contributor

carllin commented Sep 30, 2019

Problem

Work done across FEC sets for shredding isn't parallelized, leaving performance on the table.

Summary of Changes

  1. Serialize all entries
  2. Break them into shred sized chunks, generate shreds
  3. Organize shreds into FEC sets
  4. Generate coding

TODO:

  1. Test for correctness, probably lots of broken things.
  2. As part of 1) fix the tests in shred.rs

Fixes #

@carllin carllin requested review from pgarg66 and sagar-solana Sep 30, 2019
@carllin carllin added the noCI label Sep 30, 2019
@garious

This comment has been minimized.

Copy link
Member

garious commented Sep 30, 2019

The problem description doesn't describe a problem

@carllin carllin force-pushed the carllin:FixShreds branch 5 times, most recently from 44c905c to ffc2031 Oct 2, 2019
@carllin carllin removed the noCI label Oct 3, 2019
@carllin carllin force-pushed the carllin:FixShreds branch 2 times, most recently from b7a628b to ce0ee68 Oct 3, 2019
self.active_offset = 0;
self.index += 1;
// 3) Sign coding shreds
PAR_THREAD_POOL.with(|thread_pool| {

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Oct 3, 2019

Contributor

Instead of calling install on the threadpool 3 different times is there a way to structure this to call it once? You can par_iter within an install as much as you want.
The rayon bug basically kicks in after the installed closure is complete.

This comment has been minimized.

Copy link
@carllin

carllin Oct 7, 2019

Author Contributor

hmm so I tried coalescing all 3 into the same install call, but that for some reason performs marginally worse :P

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Oct 8, 2019

Contributor

Alright as long as you're confident in your data lol.

let num_shreds = ((1000 * 1000) + (shred_size - 1)) / shred_size;
// ~1Mb
let num_ticks = max_ticks_per_shred() * num_shreds as u64;
let entries = create_ticks(num_ticks, Hash::default());

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Oct 3, 2019

Contributor

May I suggest an alternative bench?

https://pastebin.com/qjVCJcNc

@carllin carllin force-pushed the carllin:FixShreds branch 14 times, most recently from 868622f to be8efff Oct 3, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 6, 2019

Codecov Report

Merging #6180 into master will increase coverage by <.1%.
The diff coverage is 88.3%.

@@           Coverage Diff            @@
##           master   #6180     +/-   ##
========================================
+ Coverage    77.9%   77.9%   +<.1%     
========================================
  Files         216     216             
  Lines       40574   40557     -17     
========================================
- Hits        31610   31597     -13     
+ Misses       8964    8960      -4
@carllin carllin force-pushed the carllin:FixShreds branch from be8efff to 11e8d42 Oct 7, 2019
@carllin carllin force-pushed the carllin:FixShreds branch 2 times, most recently from 099d79e to 907bbe7 Oct 7, 2019
@pgarg66 pgarg66 changed the title Fix shreds Shred entries in parallel Oct 7, 2019
@carllin carllin force-pushed the carllin:FixShreds branch from 907bbe7 to e51238d Oct 8, 2019
@carllin carllin marked this pull request as ready for review Oct 8, 2019
@carllin carllin merged commit ac2374e into solana-labs:master Oct 8, 2019
11 checks passed
11 checks passed
Summary 1 rule matches and 7 potential rules
Details
buildkite/solana Build #12772 passed (33 minutes, 33 seconds)
Details
buildkite/solana/bench Passed (13 minutes, 56 seconds)
Details
buildkite/solana/checks Passed (2 minutes, 1 second)
Details
buildkite/solana/coverage Passed (16 minutes, 8 seconds)
Details
buildkite/solana/local-cluster Passed (14 minutes, 12 seconds)
Details
buildkite/solana/pipeline-upload Passed (2 seconds)
Details
buildkite/solana/shellcheck Passed (25 seconds)
Details
buildkite/solana/stable Passed (14 minutes, 30 seconds)
Details
buildkite/solana/stable-perf Passed (9 minutes, 8 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.