-
Notifications
You must be signed in to change notification settings - Fork 15
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
Apply parallel sum optimization to Histogram FLP #284
Conversation
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.
Looks good, but I propose, for editorial reasons, we present Prio3SumVec first such that it immediately follows Prio3Sum. This ought to make the transitional text a little cleaner, as well being a little more logical of an order:
Prio3Count
Prio3Sum
Prio3SumVec
Prio3Hisotgram
I'd also propose that we swap the code points for Prio3SumVec and Prio3Histogram for consistency with the ordering. We're already going to need to bump the VERSION
so this shouldn't be an interop issue.
I'm wary about swapping the codepoints. If we had the IANA registries mentioned in section 10 set up, we wouldn't be able to do that. Plus, they are already baked into draft-wang-ppm-dap-taskprov. |
The IANA considerations are irrelevant until we actually ask IANA for codepoints. Similarly for taskprov: until this is a WG draft, technically we don't really care. Unless you need to support multiple versions of that draft, I'd suggest we change the codepoints here and make sure the latest version is in line. Of course, this is |
12da1d7
to
5078c3a
Compare
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.
Looks good to me. If you're concerned about changing the codepoints, feel free to leave them where they are, and I'll file issue to follow up if I decide I care enough :)
This implements #274, hoisting the description of the ParallelSum gadget up a section, and changing the Histogram circuit to use it as well. This reduces report sizes by a bit under two thirds asymptotically. Sharding performance is improved for large histograms as well (I got 27% reduction in runtime for a length of 1000)