Fewer calls to narrow and fewer steps to mess up compact gate #1056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andyleiserson
approved these changes
May 13, 2024
| secret_sharing::{replicated::semi_honest::AdditiveShare, BitDecomposed, FieldSimd}, | ||
| }; | ||
|
|
||
| #[derive(Step)] | ||
| pub enum BucketStep { | ||
| #[dynamic(256)] | ||
| #[dynamic(512)] |
There was a problem hiding this comment.
Is this MAX_BREAKDOWNS, and if so, can we use the constant here? Or if that will confuse the proc macro, at least document the association somehow?
There was a problem hiding this comment.
Yes, this should match MAX_BREAKDOWNS, but I can't use a constant here:
ipa_macros::step "dynamic" attribute expects a number of steps (<= 1024) in parentheses: #[dynamic(...)].rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B0%5D?0#file%3A%2F%2F%2FUsers%2Fbtsavage%2Fprojects%2Fipa-rust%2Fipa-core%2Fsrc%2Fprotocol%2Fipa_prf%2Faggregation%2Fbucket.rs)
There was a problem hiding this comment.
It does not seem to be supported yet? rust-lang/rust#52393
There was a problem hiding this comment.
A comment, or moving the definition of MAX_BREAKDOWNS next to the step definition, or doing const_assert_eq!(MAX_BREAKDOWNS, 512) next to the step definition, would be fine.
akoshelev
added a commit
to akoshelev/raw-ipa
that referenced
this pull request
May 13, 2024
We are working towards reducing the total number of steps (private-attribution#1056 is the step in that direction) and compact gate rearchitecture (private-attribution#961) in parallel. The old compact gate gets broken because of that but there is no real reason to fix it. To avoid red CI, this temporarily disables compact gate checks until private-attribution#961 is merged
akoshelev
added a commit
to akoshelev/raw-ipa
that referenced
this pull request
May 13, 2024
Continues the work started in private-attribution#1056 to reduce the total number of step transitions. Confirmed with @benjaminsavage that in WALR we don't need more than 9 bits and in OPRF IPA it is the same because the number of breakdowns is currently limited to 512
akoshelev
added a commit
to akoshelev/raw-ipa
that referenced
this pull request
May 13, 2024
Continues the work started in private-attribution#1056 to reduce the total number of step transitions. Confirmed with @benjaminsavage that in WALR we don't need more than 9 bits and in OPRF IPA it is the same because the number of breakdowns is currently limited to 512
akoshelev
added a commit
that referenced
this pull request
May 13, 2024
Continues the work started in #1056 to reduce the total number of step transitions. Confirmed with @benjaminsavage that in WALR we don't need more than 9 bits and in OPRF IPA it is the same because the number of breakdowns is currently limited to 512
akoshelev
added a commit
that referenced
this pull request
May 13, 2024
We are working towards reducing the total number of steps (#1056 is the step in that direction) and compact gate rearchitecture (#961) in parallel. The old compact gate gets broken because of that but there is no real reason to fix it. To avoid red CI, this temporarily disables compact gate checks until #961 is merged
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1056 +/- ##
=======================================
Coverage 90.38% 90.39%
=======================================
Files 167 167
Lines 24280 24283 +3
=======================================
+ Hits 21946 21950 +4
+ Misses 2334 2333 -1 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@akoshelev was having trouble with compact gate, because aggregation had way too many step transitions. Thing is, most of them are never used. So I found a way to use fewer "steps" and calls to "narrow" to do the same thing. This should hopefully result in a lot less steps from a compact gate perspective.