From 70cd2eff48831be7dbe80589f6a4798ef6c9b4d9 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Fri, 25 Oct 2024 14:31:59 -0700 Subject: [PATCH 1/2] Remove chunk override for attribution --- ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs b/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs index 03994ef6c..a028d8b96 100644 --- a/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs +++ b/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs @@ -501,9 +501,10 @@ where // Record IDs count users. The maximum number of multiplications per record (user) is: // (max_events - 1) * multiplictions_per_record, because the attribution circuit is // only evaluated for the second and subsequent records. - let chunk_size = TARGET_PROOF_SIZE + let chunk_size = (TARGET_PROOF_SIZE / ((histogram.len() - 1) - * multiplications_per_record::(attribution_window_seconds)); + * multiplications_per_record::(attribution_window_seconds))) + .next_power_of_two(); // Tricky hacks to work around the limitations of our current infrastructure let mut dzkp_validator = sh_ctx.clone().dzkp_validator( @@ -511,9 +512,7 @@ where protocol: &Step::Attribute, validate: &Step::AttributeValidate, }, - // TODO: this should not be necessary, but probably can't be removed - // until we align read_size with the batch size. - std::cmp::min(sh_ctx.active_work().get(), chunk_size.next_power_of_two()), + chunk_size, ); dzkp_validator.set_total_records(TotalRecords::specified(histogram[1]).unwrap()); let ctx_for_row_number = set_up_contexts(&dzkp_validator.context(), histogram)?; From 554c891e74a4da60f02a3b290790618854633d35 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Mon, 28 Oct 2024 10:09:10 -0700 Subject: [PATCH 2/2] Restore chunk override; revise comment --- ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs b/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs index a028d8b96..0df23f6c9 100644 --- a/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs +++ b/ipa-core/src/protocol/ipa_prf/prf_sharding/mod.rs @@ -501,10 +501,9 @@ where // Record IDs count users. The maximum number of multiplications per record (user) is: // (max_events - 1) * multiplictions_per_record, because the attribution circuit is // only evaluated for the second and subsequent records. - let chunk_size = (TARGET_PROOF_SIZE + let chunk_size = TARGET_PROOF_SIZE / ((histogram.len() - 1) - * multiplications_per_record::(attribution_window_seconds))) - .next_power_of_two(); + * multiplications_per_record::(attribution_window_seconds)); // Tricky hacks to work around the limitations of our current infrastructure let mut dzkp_validator = sh_ctx.clone().dzkp_validator( @@ -512,7 +511,10 @@ where protocol: &Step::Attribute, validate: &Step::AttributeValidate, }, - chunk_size, + // TODO: this override was originally added to work around problems with + // read_size vs. batch size alignment. Those are now fixed (in #1332), but this + // is still observed to help performance (see #1376), so has been retained. + std::cmp::min(sh_ctx.active_work().get(), chunk_size.next_power_of_two()), ); dzkp_validator.set_total_records(TotalRecords::specified(histogram[1]).unwrap()); let ctx_for_row_number = set_up_contexts(&dzkp_validator.context(), histogram)?;