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

feat(icicle): Add icicle MSM support #503

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

sagar-a16z
Copy link
Contributor

@sagar-a16z sagar-a16z commented Nov 15, 2024

  • Rebased feat(icicle): Add icicle msm support #322
  • Updated to latest icicle release
  • Fixed it to use the GPU correctly
  • Fall back to JOLT's CPU implementations when GPU is unavailable - much faster than the default CPU impl.
  • Added msm benchmarks
  • Overall Perf is up ~20%.

@wiz-a16z
Copy link

wiz-a16z bot commented Nov 15, 2024

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities 9 Info
Data Finding Sensitive Data
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total 9 Info

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@sagar-a16z
Copy link
Contributor Author

sagar-a16z commented Nov 15, 2024

With the newest benchmark I added, the GPU perf is actually really good. (This is a WSL machine on a 5800X3D + 3080TI)

CPU

VariableBaseMSM::msm() [mode:JOLT CPU]
                        time:   [807.29 ms 810.86 ms 814.58 ms]

GPU

VariableBaseMSM::msm() [mode:Icicle]
                        time:   [153.71 ms 156.67 ms 159.43 ms]

It’s WAY faster. For some reason it's much slower when running the examples. Maybe because my CPU usage was low while this benchmark ran where as the CPU is pinned when running the examples or maybe because I have plenty of left over memory.

@sagar-a16z sagar-a16z force-pushed the sagar/icicle_msms branch 2 times, most recently from 8bc1af1 to 705070a Compare November 19, 2024 00:11
@sagar-a16z sagar-a16z changed the title WIP feat(icicle): Add icicle MSM support feat(icicle): Add icicle MSM support Dec 2, 2024
@sagar-a16z sagar-a16z marked this pull request as ready for review December 2, 2024 21:21
Copy link
Collaborator

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

great work! main question is: I think we need to move the icicle init into prove; otherwise if someone is e.g. loading the preprocessing from disk and running prove by itself, icicle won't be initialized. wdyt?

Comment on lines +74 to +90
#[cfg(feature = "icicle")]
let gpu_bases = Some(
bases
.par_iter()
.map(|base| G1Projective::from_ark_affine(base))
.collect(),
);

let max_num_bits = scalars
.par_iter()
.map(|s| s.clone().into_bigint().num_bits())
.max()
.unwrap();

println!("Using max num bits: {}", max_num_bits);
#[cfg(not(feature = "icicle"))]
let gpu_bases = None;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "icicle")]
let gpu_bases = Some(
bases
.par_iter()
.map(|base| G1Projective::from_ark_affine(base))
.collect(),
);
let max_num_bits = scalars
.par_iter()
.map(|s| s.clone().into_bigint().num_bits())
.max()
.unwrap();
println!("Using max num bits: {}", max_num_bits);
#[cfg(not(feature = "icicle"))]
let gpu_bases = None;
#[cfg(feature = "icicle")]
let gpu_bases = Some(
bases
.par_iter()
.map(|base| G1Projective::from_ark_affine(base))
.collect(),
);
#[cfg(not(feature = "icicle"))]
let gpu_bases = None;
let max_num_bits = scalars
.par_iter()
.map(|s| s.clone().into_bigint().num_bits())
.max()
.unwrap();
println!("Using max num bits: {}", max_num_bits);

Comment on lines +59 to +66
let values: [u64; 4] = [
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
];
let bigint = ark_ff::BigInteger256::new(values);
<Fr as JoltField>::from_bytes(&bigint.to_bytes_le())
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should be able to just do

Suggested change
let values: [u64; 4] = [
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
];
let bigint = ark_ff::BigInteger256::new(values);
<Fr as JoltField>::from_bytes(&bigint.to_bytes_le())
Fr::random(&mut rng)

Comment on lines +91 to +98
let values: [u64; 4] = [
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
];
let bigint = ark_ff::BigInteger256::new(values);
<Fr as JoltField>::from_bytes(&bigint.to_bytes_le())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let values: [u64; 4] = [
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
rng.next_u64(),
];
let bigint = ark_ff::BigInteger256::new(values);
<Fr as JoltField>::from_bytes(&bigint.to_bytes_le())
Fr::random(&mut rng)

Comment on lines +318 to +321
//TODO(sagar): This should be moved to a more appropriate place - icicle makes a network request
// which impacts prover time.
icicle::icicle_init();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be moved to prove?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants