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(hash): Partially mitigate WASM memory leak #786 using FinalizationRegistry #998

Closed
wants to merge 2 commits into from
Closed

Conversation

jeremyBanks
Copy link
Contributor

As described in #786, the createHash function currently leaks memory every time it's used: the Hasher instance allocated in the WASM heap is never freed.

This change uses FinalizationRegistry (which is working correctly on deno@main as of today 🎉) to .free() the Hasher instances in WASM when their corresponding Hash JavaScript objects are garbage collected. This does not entirely resolve the memory leak: the WASM heap will never shrink and release the used memory, but it can significantly mitigate it. In a test on my machine, endlessly calling createHash('sha256'), the WASM heap grew more slowly, and stopped growing around ~70MB, as the allocator inside the WASM was able to reuse the heap space we freed. Without this change, the memory use continued to grow without bound; I stopped it after 1GB.

This won't be effective in all cases: The garbage collector may not get a chance to run if you're repeatedly creating hashes in a hot sync loop. If you're passing huge chunks of data into WASM, the heap will still need to be expanded to hold them (discussed in Discord). But I think it's still a significant improvement.

It appears that wasm-bindgen does have an option that would allow it to do this automatically internally, but it hasn't been exposed through wasm-pack that we're using here, and it's not clear that it will be soon.


Test script:

#!/usr/bin/env -S deno run --unstable --allow-all
import { createHash } from "./mod.ts";
import { Hash, _wasm } from "./_wasm/hash.ts";


for (let i = 0; i < 1_000_000_000; i++) {
    createHash("sha512");

    if (i % 10_000 === 0) {
      // Yield thread, making it more likely for the GC to run.
      await new Promise((resolve) => setTimeout(resolve, 1));
    }

    if (i % 100_000 === 0) {
      console.log(`
        ${i} hashes created
        ${_wasm.memory.buffer.byteLength} bytes used by WASM`)
    }
  }

with

export const _wasm = await init(source);

added in hash.ts to let us check the memory use.

@jeremyBanks jeremyBanks marked this pull request as draft July 2, 2021 17:16
@jeremyBanks
Copy link
Contributor Author

This seems to be affecting the exit status of a test subprocess in an unexpected way. Converting to draft since this obviously isn't suitable for merge as-is.

…nRegistry

This change uses `FinalizationRegistry` (which is working correctly on deno@main as of today) to .free() the Hasher instances in WASM when their corresponding `Hash` JavaScript objects are garbage collected. This does **not** entirely resolve the memory leak: the WASM heap will never shrink and release the used memory, but it can significantly mitigate it. In a test on my machine, endlessly calling `createHash('sha256')`, the WASM heap grew more slowly, and stopped growing around ~70MB, as the allocator inside the WASM was able to reuse the heap space we freed. Without this change, the memory use continued to grow without bound; I stopped it after 1GB.

This won't be effective in all cases: The garbage collector may not get a chance to run if you're repeatedly creating hashes in a hot sync loop. If you're passing huge chunks of data into WASM, the heap will still need to be expanded to hold them. But I think it's still a significant improvement.

It appears that wasm-bindgen does have an option that would allow it to do this automatically internally, but it hasn't been exposed through wasm-pack that we're using here, and it's not clear that it will be soon.
@jeremyBanks
Copy link
Contributor Author

It looks like the type error was caused by my using a #private static property, which TypeScript and SWC supported differently. Switched to a module variable and it seems to be working fine, with tests passing and stable memory use in the latest Deno canary.

@jeremyBanks jeremyBanks marked this pull request as ready for review July 2, 2021 19:22
@jeremyBanks
Copy link
Contributor Author

jeremyBanks commented Jul 2, 2021

@caspervonb suggested we might want to just switch away from wasm-pack. That would make this redundant with wasm-bindgen's automatic capability to insert finalization logic. Closing this in favour of exploring that.

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.

1 participant