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

fix: avoid one in memory copy for unsealing #1401

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Jan 25, 2021

Refactors extraction to overwrite the provided data, rather than allocating another one.

@cryptonemo
Copy link
Collaborator

This is a memory visualization of a full 32GiB seal lifecycle test using the code from this PR, with unseal starting around 10pm.

unseal-improvements-01-26-2021

For comparison, this is a partial memory visualization of a 32GiB seal lifecycle test on the master branch, with unseal starting around 1:28pm. Note that a large part of the test visualization at the start was unfortunately lost -- and that this run is not yet complete.

Screenshot(120)

@cryptonemo
Copy link
Collaborator

cryptonemo commented Jan 27, 2021

To most directly take advantage of this, we also need the exposed unseal_range method in rust-filecoin-proofs-api to be re-factored in terms of get_unsealed_range.

https://github.com/filecoin-project/rust-filecoin-proofs-api/blob/master/src/seal.rs#L843

The unseal_range API call does not directly benefit from the memory reduction via mmap. It's possible that lotus does not use unseal_range in preference to get_unsealed_range, but it should be updated in any case since it can be done transparently to the API caller and the memory performance is strictly reduced.

EDIT: Still looking at the possibility on this. It may just be recommended that callers don't use unseal_range and it becomes a deprecated call. Anyone able to call it can alternatively call get_unsealed_range.

EDIT 2: It looks possible to leave the rust-filecoin-proofs-api repo alone and instead re-factor how unseal is called from the filecoin-ffi repo.

EDIT 3: Something like this should do it: filecoin-project/filecoin-ffi#162

@cryptonemo
Copy link
Collaborator

This is the latest run, with the updated/correct code. This uses the blst,gpu2 feature, so you can see that tree building is much more efficient in terms of time and memory. Unsealing starts around 4:41am

api-1956d861dd69f895_418389

@cryptonemo
Copy link
Collaborator

And here is the latest run of master, comparable to the improved image above.

api-1956d861dd69f895_422637

dignifiedquire and others added 3 commits March 19, 2021 07:43
unseal_range is not readily able to be converted, but we should be
able to deprecate this call from -api by having unseal call into
get_unsealed_range transparently to the caller
Copy link
Contributor Author

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (can't approve my own PR)

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 this pull request may close these issues.

2 participants