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

build(hash): switch from wasm-pack to using wasm-bindgen directly #999

Merged
merged 35 commits into from
Jul 7, 2021
Merged

build(hash): switch from wasm-pack to using wasm-bindgen directly #999

merged 35 commits into from
Jul 7, 2021

Conversation

jeremyBanks
Copy link
Contributor

@jeremyBanks jeremyBanks commented Jul 3, 2021

wasm-pack limits our ability to configure wasm-bindgen and is no longer necessary as we can use wasm-bindgen-cli directly (as @caspervonb pointed out). This updates some rust dependencies and removes our use of wasm-pack. The logic to rebuild the WASM is added to the our GitHub Actions ci.yaml.

This PR also modifies the inlined WASM data to insert line breaks every 78 characters, so that it's split across lines instead of being on a single line. Putting it all on a single line can cause performance issues for some debugging or source control tools. It also can give us more useful diffs in some cases: you can see in cb3d9fb that there are some changes to the file when regenerated it, but they are small and don't change the binary size.

Using wasm-bindgen directly allows us to enable --weakrefs, which partially mitigates #786 by using a FinalizationRegistry to free Hasher instances in the WASM heap 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 ~30MB, 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. (The test script I used is posted in #998.)

This mitigation 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 it's still a significant improvement.

wasm-pack limits our ability to configure wasm-bindgen and is no
longer necessary as we can use wasm-bindgen-cli directly. This
allows us to enable --weakrefs which partially mitigates #786 by
using a FinalizationRegistry (currently working in Deno Canary,
though not yet on Stable) to free memory in the WASM heap
automatically when the corresponding JavaScript wrapper objects
are garbage-collected.
@wperron wperron self-requested a review July 4, 2021 15:30
@wperron wperron self-assigned this Jul 4, 2021
.github/workflows/wasm.yml Outdated Show resolved Hide resolved
.github/workflows/wasm.yml Outdated Show resolved Hide resolved
hash/_wasm/build.ts Show resolved Hide resolved
@wperron
Copy link
Contributor

wperron commented Jul 5, 2021

I'm also unsure about the github action to rebuild the wasm binary, on the one hand it's nice to have it automated and standardized but on the other hand I'm sure we could simply have a build script and have one of the core maintainers do it manually, after all we don't get that many PRs on the hash module. @bartlomieju wdyt?

@bartlomieju
Copy link
Member

I'm also unsure about the github action to rebuild the wasm binary, on the one hand it's nice to have it automated and standardized but on the other hand I'm sure we could simply have a build script and have one of the core maintainers do it manually, after all we don't get that many PRs on the hash module. @bartlomieju wdyt?

I think it'd be better to have that script integrated into CI otherwise it might quickly go stale and don't work when upgrade time comes up.

@wperron
Copy link
Contributor

wperron commented Jul 5, 2021

I think it'd be better to have that script integrated into CI otherwise it might quickly go stale and don't work when upgrade time comes up.

So a job that runs when the PR is opened so we don't have to think about it as we can be sure it's there when we merge? that's a good idea, I like that 👍

.github/workflows/wasm.yml Outdated Show resolved Hide resolved
@jeremyBanks
Copy link
Contributor Author

I've merged the WASM Action into the existing ci.yaml as @wperron suggested. The logic is now:

  1. Check if there were any changes to hash/ in the latest commit (per git diff HEAD~ -- ./hash/_wasm). If not, stop.
  2. Install the WASM toolchain dependencies.
  3. Rebuild the WASM, bindings, etc.
  4. Check whether the rebuilt code has any changes. If not, stop.
  5. Run the wasm/ tests again (only on this Linux build server, not the whole matrix) to ensure that the new build hasn't broken them. If they fail, stop.
  6. Push the changes up to the branch.

Commits that are pushed from GitHub actions do not themselves trigger GitHub actions, hence the need to re-test them ourselves (#5 above).

I've also eliminated the non-determinism we were seeing in the builds (it was a build path in an error string), at least while building on Linux, so this shouldn't create commits unnecessarily.

@jeremyBanks jeremyBanks requested a review from wperron July 6, 2021 01:07
@jeremyBanks
Copy link
Contributor Author

jeremyBanks commented Jul 6, 2021

🤦 It looks like there isn't any (reasonable) way to push changes back to a source branch that's in a different repository (i.e., most PRs). Oops. It had seemed to be working because when testing I was running it on push within my fork.

But if the build is reproducible and stable, it should be enough to rebuild and verify on GitHub, to make sure that the output matches what was committed by the developer (sort-of like deno fmt --check), so I've updated the action to just do that instead (example verification failure and verification success).

@jeremyBanks jeremyBanks requested a review from wperron July 6, 2021 16:50
hash/_wasm/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@wperron wperron left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@wperron wperron merged commit e0ec201 into denoland:main Jul 7, 2021
@jeremyBanks jeremyBanks deleted the wasm-bindgen branch July 7, 2021 18:50
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.

3 participants