Skip to content
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

Helper peak memory usage can be reduced #3285

Closed
divergentdave opened this issue Jul 9, 2024 · 1 comment · Fixed by #3303
Closed

Helper peak memory usage can be reduced #3285

divergentdave opened this issue Jul 9, 2024 · 1 comment · Fixed by #3303
Assignees

Comments

@divergentdave
Copy link
Collaborator

I set up a harness to run one big aggregation job under Valgrind, using the DHAT tool. For both the leader and the helper, the peak memory usage was similar, double the total measurement share/output share size, plus various smaller allocations. (in the case of the helper, the next largest allocation is the aggregation job initialization request body, in a trillium_http::ReceivedBody)

In the case of the leader, one copy of the measurement share is allocated when loading report aggregations from the database in the aggregation job driver. The other copy is made inside the VDAF's prepare_init(), because the Prio3 leader measurement share is cloned for storage in Prio3PrepareState. We could probably eliminate this copy with Arcs inside the Prio3 implementation internals, in order to share and then transfer ownership of this vector, but that's outside the scope of this repository.

In the case of the helper, peak memory usage occurs after VDAF preparation, when preparing to write results to the database. The largest allocations come from two copies of the output shares. One is allocated in Flp::truncate(), and the other is allocated when cloning a Vec<ReportShareData> (which contains in turn WritableReportAggregation and A::OutputShare). This vector is cloned for two reasons, to give an async block ownership while still allowing for transaction retries, and to allow the vector to be modified inside the transaction before the report aggregations are finally written out. One of these copies could be eliminated if the original Vec<ReportShareData> were moved into an Arc, with ownership shared between each retry of the transaction, and if the mid-transaction data structure updates were done on a Vec<Cow<'_, _>>, with borrow references pointing back to the original Arc<Vec<ReportShareData>>. This would require an involved refactor, to allow AggregationJobWriter to work with Cows for both phases of operation, instead of only its second phase, but it could cut peak memory usage by almost half, giving us more headroom from OOM errors.

@branlwyd
Copy link
Contributor

branlwyd commented Jul 9, 2024

I think this is a worthwhile optimization -- I noticed the copying but did not measure to realize it would be our largest allocation. I may be able to prioritize it soon unless you plan to take on the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants