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

Enable Wasm weak references for automatic garbage collection #694

Merged
merged 14 commits into from
Mar 9, 2022

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Mar 7, 2022

Description of change

Enable Weak References in the Wasm bindings. This ensures that Wasm memory is freed automatically by the garbage collector, otherwise it will continue to grow unless the developer explicitly calls .free() on every exported instance or the instance is passed back to a function which takes ownership of it, allowing Rust to free it.

Unfortunately wasm-pack does not support passing the --weak-refs flag to wasm-bindgen (issue), so we have to invoke cargo, wasm-bindgen, and wasm-opt manually.

Another downside is that wasm-bindgen currently has a bug where parameters passed by value fail to deregister Finalizer handlers. This causes runtime errors from automatic garbage collection trying to free the instance when it is no longer referenced from the Javascript side, since WasmRefCell sees the instance as still borrowed.
See: rustwasm/wasm-bindgen#2677

Fortunately this is easy-enough to circumvent by borrowing and cloning owned parameters instead. I added a build-time lint to prevent this problem going forward until the above PR is merged.

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Wasm tests and examples pass locally. I checked the memory usage locally with Chrome dev tools to confirm that the memory is freed by automatic garbage collection now.

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Enable weak references with --weak-refs.
Fix garbage collection runtime alias errors due to weak references.
Add lint to prevent weak reference alias errors.
@cycraig cycraig added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Patch Change without affecting the API that requires a patch release. Part of "Patch" section in changelog labels Mar 7, 2022
@cycraig cycraig added this to the v0.5 Features milestone Mar 7, 2022
@cycraig cycraig self-assigned this Mar 9, 2022
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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 to me! Good idea with the lint, too.

@@ -57,6 +57,7 @@
"jsdoc-to-markdown": "^7.1.1",
"mocha": "^9.2.0",
"txm": "^8.0.1",
"wasm-opt": "^1.3.0",
"wasm-pack": "0.10.1",
Copy link
Collaborator

@eike-hass eike-hass Mar 9, 2022

Choose a reason for hiding this comment

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

Since we don't use wasm-pack anymore, I assume this can be removed.

Copy link
Contributor Author

@cycraig cycraig Mar 9, 2022

Choose a reason for hiding this comment

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

We still invoke wasm-pack for running the unit tests (it's more complicated to set up that environment apparently).

@eike-hass
Copy link
Collaborator

eike-hass commented Mar 9, 2022

Other then the two minor points, LGTM! Will accept now already.
Let me know when it is ready, then we can admin merge (because of the windows runner issue).

@eike-hass eike-hass self-requested a review March 9, 2022 14:58
@cycraig cycraig merged commit 424ba5e into dev Mar 9, 2022
@cycraig cycraig deleted the feat/wasm-weak-refs branch March 9, 2022 17:03
@cycraig cycraig changed the title Enable Wasm weak references Enable Wasm weak references for automatic garbage collection Mar 17, 2022
@yisibl
Copy link

yisibl commented Mar 25, 2024

Starting with v0.2.91 "wasm-bindgen does use the TC39 weak references proposal if support is detected. At the time of this writing all major browsers do support it."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Patch Change without affecting the API that requires a patch release. Part of "Patch" section in changelog Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
Development

Successfully merging this pull request may close these issues.

4 participants