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.

Sign up for GitHub

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

Jump to bottom

Update support for weak references #2248

Merged
merged 2 commits into from
Jul 23, 2020
Merged

Update support for weak references #2248

merged 2 commits into from
Jul 23, 2020

Conversation

alexcrichton
Copy link
Contributor

This commit updates the WASM_BINDGEN_WEAKREF feature support to the
latest version of the weak references proposal in JS. This also updates
the Closure type to use finalization registries to ensure closures are
deallocated with the JS gc. This means that Closure::forget will not
actually leak memory in a weakref-using application.

@alexcrichton
Update support for weak referenes
c5635ce 
This commit updates the `WASM_BINDGEN_WEAKREF` feature support to the
latest version of the weak references proposal in JS. This also updates
the `Closure` type to use finalization registries to ensure closures are
deallocated with the JS gc. This means that `Closure::forget` will not
actually leak memory in a weakref-using application.
@alexcrichton alexcrichton requested a review from fitzgen July 22, 2020 17:02
fitzgen
fitzgen approved these changes Jul 23, 2020
View reviewed changes
src/closure.rs
/// will not leak memory. Instead the Rust memory will be reclaimed when the
/// JS closure is GC'd. Weak references are not enabled by default since
/// they're still a proposal for the JS standard. They can be enabled with
/// `WASM_BINDGEN_WEAKREF=1` when running `wasm-bindgen`, however.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe time to make an actual CLI flag, now that these are pretty far down the standards pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's worth turning this on by default and requiring a flag to turn it off? I'm not actually sure at what point we should do that...

I agree though that at least this should be a CLI flag now.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we are at default-on yet. Probably not for a year or so, I'd guess, if ever :-/

@alexcrichton alexcrichton changed the title Update support for weak referenes Update support for weak references Jul 23, 2020
@alexcrichton alexcrichton merged commit 664c3f8 into rustwasm:master Jul 23, 2020
@alexcrichton alexcrichton deleted the weakref branch July 23, 2020 20:05
@alexcrichton alexcrichton mentioned this pull request Jul 23, 2020
Closed
@kellpossible kellpossible mentioned this pull request Jul 28, 2020
Closed
@kellpossible
Copy link

kellpossible commented Jul 28, 2020 •
edited
Loading

Just a heads up, this change broke the gloo-events crate: rustwasm/gloo#120 (sorry for the duplicate of the comment on closure.rs, but it says pending, so I wasn't sure if anyone would see it)

@alexcrichton
Copy link
Contributor Author

Thanks for the report! I'll publish a new release with #2258 to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen fitzgen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexcrichton @kellpossible @fitzgen