Skip to content

Conversation

pdroalves
Copy link
Contributor

@pdroalves pdroalves commented Oct 13, 2025

closes: https://github.com/zama-ai/tfhe-rs-internal/issues/1166

PR content/description

Check-list:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Relevant issues are marked as resolved/closed, related issues are linked in the description
  • Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

This change is Reviewable

@cla-bot cla-bot bot added the cla-signed label Oct 13, 2025
@pdroalves pdroalves changed the title fix(gpu): fix 128-bit compression benchmark fix(gpu): fix 128-bit compression throughput benchmark Oct 13, 2025
@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch 6 times, most recently from 24ea477 to fa60ad4 Compare October 13, 2025 13:11
@pdroalves pdroalves changed the title fix(gpu): fix 128-bit compression throughput benchmark fix(gpu): fix compression throughput benchmark Oct 13, 2025
@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch from fa60ad4 to 0194aad Compare October 13, 2025 17:24
@pdroalves pdroalves marked this pull request as ready for review October 13, 2025 17:26
@pdroalves pdroalves requested a review from agnesLeroy October 13, 2025 17:27
@pdroalves
Copy link
Contributor Author

@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.

@pdroalves
Copy link
Contributor Author

@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.

@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch 3 times, most recently from 21066f7 to 247b2c3 Compare October 13, 2025 19:21
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;
Copy link
Contributor

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.

Copy link
Member

@IceTDrinker IceTDrinker left a 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

@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch 9 times, most recently from 08395f9 to bf0e4a9 Compare October 16, 2025 15:25
@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch 2 times, most recently from f4be469 to 9180920 Compare October 16, 2025 15:51
@pdroalves pdroalves force-pushed the pa/fix/128b-compression-bench branch from 9180920 to ae807d8 Compare October 16, 2025 19:18
@IceTDrinker
Copy link
Member

let me know if my review is blocking here

@pdroalves
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants