-
Notifications
You must be signed in to change notification settings - Fork 280
fix(gpu): fix compression throughput benchmark #2886
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
base: main
Are you sure you want to change the base?
Conversation
24ea477
to
fa60ad4
Compare
fa60ad4
to
0194aad
Compare
@agnesLeroy We still need to validate the new numbers of elements for 64 and 128-bit compression. These values are good for 4xL40 but I wasn't able to launch SXM5 to see how it goes there. |
@soonum This PR splits 64 and 128-bit compression benchmarks. This way we are more consistent (as we do with PBS) and it's faster to bench 128-b compression. Otherwise we have to wait forever for the 64-bit compression be benchmarked, discarded, and only then run 128-b. |
21066f7
to
247b2c3
Compare
let num_block = (bit_size as f64 / (param.message_modulus.0 as f64).log(2.0)) | ||
.ceil() as usize; | ||
let elements = throughput_num_threads(num_block, pbs_count); | ||
let elements = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These benches will only run on SXM5 so we need to fine tune this number for there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover logging in tfhe-zk-pok needs to be removed
a rename would be nice to match the PR https://reviewable.io/reviews/zama-ai/tfhe-rs/2814 from David that reworks some benchmarks to be easier to use for perf regressions
@IceTDrinker reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pdroalves and @soonum)
tfhe-zk-pok/src/curve_api/bls12_446.rs
line 317 at r1 (raw file):
#[track_caller] pub fn multi_mul_scalar(bases: &[Self], scalars: &[Zp]) -> G2 { let start = Instant::now();
needs to be removed
tfhe-zk-pok/src/curve_api/bls12_446.rs
line 329 at r1 (raw file):
}; println!( "multi_mul_scalar: {} bases, {} scalars, {:.2} ms",
same here
tfhe-benchmark/Cargo.toml
line 100 at r1 (raw file):
[[bench]] name = "glwe_packing_compression_128b-integer-bench"
naming to be checked
Makefile
line 1380 at r1 (raw file):
RUSTFLAGS="$(RUSTFLAGS)" __TFHE_RS_BENCH_TYPE=$(BENCH_TYPE) \ cargo $(CARGO_RS_CHECK_TOOLCHAIN) bench \ --bench glwe_packing_compression_128b-integer-bench \
there is a renaming in progress @soonum in your PR if you can advise on the "correct" naming here so that both merges are in accordance
08395f9
to
bf0e4a9
Compare
f4be469
to
9180920
Compare
9180920
to
ae807d8
Compare
let me know if my review is blocking here |
@IceTDrinker not really. I'm still trying to find the optimal number of elements for each bit size. With the scarcity of sxm5s and another task I'm working on, it's taking some time. |
closes: https://github.com/zama-ai/tfhe-rs-internal/issues/1166
PR content/description
Check-list:
This change is