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.

Sign up for GitHub

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

Jump to bottom

perf: VID ADVZ verify_share use parallelism over multiplicity #650

Merged
merged 8 commits into from
Aug 14, 2024
Merged

perf: VID ADVZ verify_share use parallelism over multiplicity #650

merged 8 commits into from
Aug 14, 2024

Conversation

ggutoski
Copy link
Contributor

closes: #646

This PR does / key places to review:

  • use parallelism here
  • new benchmark in vid/benches/advz_multiplicity.rs demonstrates perf improvement
  • My local environment disallows me from testing whether these changes break the build when gpu-vid feature is enabled. Last time this issue arose @mrain had to check the build using his nvidia gpu.

How to test this PR:

From inside the vid/ directory run

RAYON_NUM_THREADS=N cargo bench --bench=advz_multiplicity --features="test-srs"

On my local machine I observe significant improvement when I increase N from 1 to 6:

N=1:

advz_verify_payload_256KB_multiplicity/1
                        time:   [28.000 ms 29.097 ms 30.711 ms]
advz_verify_payload_256KB_multiplicity/256
                        time:   [364.81 ms 367.66 ms 371.66 ms]

N=6:

advz_verify_payload_256KB_multiplicity/1
                        time:   [27.515 ms 27.925 ms 28.442 ms]
                        change: [-5.5630% -2.1819% +0.6662%] (p = 0.24 > 0.05)
                        No change in performance detected.
advz_verify_payload_256KB_multiplicity/256
                        time:   [87.886 ms 89.704 ms 92.870 ms]
                        change: [-75.541% -74.950% -74.290%] (p = 0.00 < 0.05)
                        Performance has improved.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@ggutoski ggutoski requested review from mrain and akonring August 12, 2024 21:10
akonring
akonring reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@akonring akonring left a comment

Choose a reason for hiding this comment

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

LGTM.
Added a couple of nit comments and it would be good to test this on GPU as well.
Really nice catch that we should make the verification (with multiplicity) parallelizable as well.

vid/benches/advz_multiplicity.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/benches/advz_multiplicity.rs Outdated Show resolved Hide resolved
@akonring
Copy link
Contributor

Will let @mrain (or others) approve this since I don't have access to GPU. Otherwise, we could setup an ephemeral GPU server to test this.

@akonring
Copy link
Contributor

And what's up with all the clippy warnings(?)

mrain
mrain previously approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@mrain mrain left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM. GPU tests are good.

nit: 2 VID tests in advz::bytes_to_field are not working. Try cargo test -p jf-vid --features test-srs --release -- --ignored for details.

@ggutoski
Copy link
Contributor Author

nit: 2 VID tests in advz::bytes_to_field are not working. Try cargo test -p jf-vid --features test-srs --release -- --ignored for details.

These doctests are ignored intentionally due to a Rust limitation that disallows access to private identifiers in doctest: Doctest of private functions - help - The Rust Programming Language Forum 😕

@ggutoski
Copy link
Contributor Author

And what's up with all the clippy warnings(?)

Github CI is using a newer version of Rust than our Nix environment. We need to update Nix flake again. We'll do that in another PR.

@ggutoski ggutoski requested a review from akonring August 13, 2024 16:35
akonring
akonring approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@akonring akonring left a comment

Choose a reason for hiding this comment

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

Nice.
Some numbers below:

jellyfish$ RAYON_NUM_THREADS=10 cargo bench --bench=advz_multiplicity --features="test-srs"
    Finished `bench` profile [optimized] target(s) in 0.32s
     Running benches/advz_multiplicity.rs (target/nix_rustc/release/deps/advz_multiplicity-b08a1b7a37662771)
advz_verify_payload_256KB_multiplicity/1
                        time:   [21.979 ms 22.338 ms 22.523 ms]
                        change: [-0.9804% +0.5734% +2.1592%] (p = 0.51 > 0.05)
                        No change in performance detected.
advz_verify_payload_256KB_multiplicity/256
                        time:   [32.937 ms 33.757 ms 35.228 ms]
                        change: [-6.8922% -1.3409% +4.3643%] (p = 0.67 > 0.05)
                        No change in performance detected.

@ggutoski ggutoski merged commit e18f3bc into main Aug 14, 2024
5 checks passed
@ggutoski ggutoski deleted the gg/646 branch August 14, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akonring akonring akonring approved these changes

@mrain mrain mrain left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use parallelism over multiplicity in ADVZ verify_share
3 participants
@ggutoski @akonring @mrain