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

Use a sparse array in DecryptNotesResponse #5128

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

andiflabs
Copy link
Contributor

@andiflabs andiflabs commented Jul 10, 2024

Summary

Most of the note decryption will fail. Currently, if a DecryptNotesResponse contains 100 undecrypted notes, it will create a dense array containing 100 null values, causing unnecessary large memory allocations and large memory writes. It was in fact observed that the arrays from DecryptNotesResponse were one of the top contributors to memory usage during wallet scans.

This commit changes the implementation so that now DecryptNotesResponse would create a sparse array containing no values at all. This means that, when no notes are decrypted (i.e. the most common case), all DecryptNotesResponse objects consume the same amount of memory, regardless of how many accounts are in the wallet, or how many encrypted notes were requested.

This was shown to reduce the peak memory usage of the node process, as well as reduce the number of minor page faults.

Testing Plan

Documentation

N/A

Breaking Change

N/A

@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from d47ad02 to 9aa2688 Compare July 10, 2024 23:49
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from 6f3dd00 to 148f41b Compare July 11, 2024 16:41
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from 9aa2688 to d29c962 Compare July 11, 2024 16:41
@andiflabs andiflabs marked this pull request as ready for review July 11, 2024 16:41
@andiflabs andiflabs requested a review from a team as a code owner July 11, 2024 16:41
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from 148f41b to 8e8a2b7 Compare July 11, 2024 16:55
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from d29c962 to b4fe7d0 Compare July 11, 2024 16:55
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from 8e8a2b7 to f280136 Compare July 11, 2024 19:31
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from b4fe7d0 to 720b052 Compare July 11, 2024 20:32
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from f280136 to abc46be Compare July 11, 2024 20:54
Copy link
Contributor

@hughy hughy left a comment

Choose a reason for hiding this comment

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

Looks good, tested locally and scanning seems to work correctly

I think we might benefit from more test coverage that addresses an example of a sparse response

ironfish/src/workerPool/tasks/decryptNotes.ts Outdated Show resolved Hide resolved
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from abc46be to 1dcff5a Compare July 11, 2024 22:12
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from 720b052 to 403f126 Compare July 11, 2024 22:16
@andiflabs andiflabs force-pushed the andrea/worker-pool-transfer-and-shared-mem branch from 1dcff5a to 930aa7f Compare July 11, 2024 22:20
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from 403f126 to f8763fc Compare July 11, 2024 22:20
Base automatically changed from andrea/worker-pool-transfer-and-shared-mem to staging July 11, 2024 23:15
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from f8763fc to 8c4ec51 Compare July 11, 2024 23:16
Most of the note decryption will fail. Currently, if a
`DecryptNotesResponse` contains 100 undecrypted notes, it will create a
dense array containing 100 `null` values, causing unnecessary large
memory allocations and large memory writes. It was in fact observed that
the arrays from `DecryptNotesResponse` were one of the top contributors
to memory usage during wallet scans.

This commit changes the implementation so that now
`DecryptNotesResponse` would create a sparse array containing no values
at all. This means that, when no notes are decrypted (i.e. the most
common case), all `DecryptNotesResponse` objects consume the same amount
of memory, regardless of how many accounts are in the wallet, or how
many encrypted notes were requested.

This was shown to reduce the peak memory usage of the node process, as
well as reduce the number of minor page faults.
@andiflabs andiflabs force-pushed the andrea/decrypt-notes-response-sparse-array branch from 8c4ec51 to dad63ba Compare July 15, 2024 21:32
@andiflabs andiflabs merged commit dad63ba into staging Jul 15, 2024
10 checks passed
@andiflabs andiflabs deleted the andrea/decrypt-notes-response-sparse-array branch July 15, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants