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

perf(hash): Chunk message input in JavaScript to reduce WASM heap size #1010

Merged
merged 3 commits into from
Jul 9, 2021
Merged

perf(hash): Chunk message input in JavaScript to reduce WASM heap size #1010

merged 3 commits into from
Jul 9, 2021

Conversation

jeremyBanks
Copy link
Contributor

@jeremyBanks jeremyBanks commented Jul 7, 2021

It is generally not possible to reduce the size of a WASM module's heap, only grow it (pointed out by
@evanwashere in Discord, see also WebAssembly/design#1300). There are some possible work-arounds, but they would add complexity and might not be possible with our current wasm-bindgen/wasm-pack build approach.

When you .update(...) our WASM Hasher with a message, that data needs to be copied over to the WASM heap, which may require it to grow if it doesn't have enough available space. So if you pass a message that's 10MB, or 2GB, the WASM heap and memory usage will increase to at least that much and never come down.

This change instead splits messages into chunks of up to 64KB each and feeds them to WASM individually, keeping the total WASM heap size under a few megabytes regardless of the message size.

I benchmarked this to ensure it wouldn't adversely affect throughput, and found that as long as the piece size isn't too small, throughput is very-slightly improved relative to the existing code. I used 64KB chunks because they seemed the smallest fastest chunk size in my testing, by a small margin.

@kt3k
Copy link
Member

kt3k commented Jul 8, 2021

@jeremyBanks This seems like a decent fix to me, but is there any way to check this actually reduces the resulted heap size? Ideally we'd like to have a test case for checking it.

@jeremyBanks
Copy link
Contributor Author

jeremyBanks commented Jul 8, 2021

@kt3k Yes, we can check wasm.memory.buffer.byteLength. I had an ad hoc test script but it wasn't a clean unit test so I didn't commit it. I'll see if I can fix it up later (I guess I'll need to run the tests in subprocesses to reset the heap between them.)

@jeremyBanks
Copy link
Contributor Author

@kt3k I have added a test for memory use, ensuring that it stays under 2MB when provided with a 64MB input. Without the change, it would fail with this message:

AssertionError: WASM heap was too large after large input: 65.2 MB

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@jeremyBanks Great! LGTM. Thanks!

@kt3k kt3k merged commit 52e42d6 into denoland:main Jul 9, 2021
@jeremyBanks jeremyBanks deleted the perf-hash branch July 9, 2021 12:13
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