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

Update support for weak references #2248

Merged
merged 2 commits into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ jobs:
# displayName: "(anyref) Crate test suite (no debug)"
# - script: cargo test --target wasm32-unknown-unknown --features serde-serialize --test wasm
# displayName: "(anyref) Crate test suite (with serde)"
- script: |
set -e
echo "##vso[task.setvariable variable=NODE_ARGS]--harmony-weak-refs"
echo "##vso[task.setvariable variable=WASM_BINDGEN_WEAKREF]1"
displayName: "Configure anyref passes"
- script: cargo test --target wasm32-unknown-unknown --test wasm
displayName: "(weakref) Crate test suite"
- script: WASM_BINDGEN_NO_DEBUG=1 cargo test --target wasm32-unknown-unknown --test wasm
displayName: "(weakref) Crate test suite (no debug)"
- script: cargo test --target wasm32-unknown-unknown --features serde-serialize --test wasm
displayName: "(weakref) Crate test suite (with serde)"

- job: test_wasm_bindgen_windows
displayName: "Run wasm-bindgen crate tests (Windows)"
Expand Down
3 changes: 0 additions & 3 deletions crates/cli-support/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ intrinsics! {
#[symbol = "__wbindgen_cb_drop"]
#[signature = fn(Externref) -> Boolean]
CallbackDrop,
#[symbol = "__wbindgen_cb_forget"]
#[signature = fn(Externref) -> Unit]
CallbackForget,
#[symbol = "__wbindgen_number_new"]
#[signature = fn(F64) -> Externref]
NumberNew,
Expand Down
79 changes: 58 additions & 21 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ impl<'a> Context<'a> {
",
name,
if self.config.weak_refs {
format!("{}FinalizationGroup.register(obj, obj.ptr, obj.ptr);", name)
format!("{}Finalization.register(obj, obj.ptr, obj);", name)
} else {
String::new()
},
Expand All @@ -790,13 +790,7 @@ impl<'a> Context<'a> {

if self.config.weak_refs {
self.global(&format!(
"
const {}FinalizationGroup = new FinalizationGroup((items) => {{
for (const ptr of items) {{
wasm.{}(ptr);
}}
}});
",
"const {}Finalization = new FinalizationRegistry(ptr => wasm.{}(ptr));",
name,
wasm_bindgen_shared::free_function(&name),
));
Expand Down Expand Up @@ -860,7 +854,7 @@ impl<'a> Context<'a> {
}}
",
if self.config.weak_refs {
format!("{}FinalizationGroup.unregister(ptr);", name)
format!("{}Finalization.unregister(this);", name)
} else {
String::new()
},
Expand Down Expand Up @@ -1938,6 +1932,16 @@ impl<'a> Context<'a> {

let table = self.export_function_table()?;

let (register, unregister) = if self.config.weak_refs {
self.expose_closure_finalization()?;
(
"CLOSURE_DTORS.register(real, state, state);",
"CLOSURE_DTORS.unregister(state)",
)
} else {
("", "")
};

// For mutable closures they can't be invoked recursively.
// To handle that we swap out the `this.a` pointer with zero
// while we invoke it. If we finish and the closure wasn't
Expand All @@ -1946,7 +1950,7 @@ impl<'a> Context<'a> {
self.global(&format!(
"
function makeMutClosure(arg0, arg1, dtor, f) {{
const state = {{ a: arg0, b: arg1, cnt: 1 }};
const state = {{ a: arg0, b: arg1, cnt: 1, dtor }};
const real = (...args) => {{
// First up with a closure we increment the internal reference
// count. This ensures that the Rust closure environment won't
Expand All @@ -1957,15 +1961,22 @@ impl<'a> Context<'a> {
try {{
return f(a, state.b, ...args);
}} finally {{
if (--state.cnt === 0) wasm.{}.get(dtor)(a, state.b);
else state.a = a;
if (--state.cnt === 0) {{
wasm.{table}.get(state.dtor)(a, state.b);
{unregister}
}} else {{
state.a = a;
}}
}}
}};
real.original = state;
{register}
return real;
}}
",
table
table = table,
register = register,
unregister = unregister,
));

Ok(())
Expand All @@ -1978,6 +1989,16 @@ impl<'a> Context<'a> {

let table = self.export_function_table()?;

let (register, unregister) = if self.config.weak_refs {
self.expose_closure_finalization()?;
(
"CLOSURE_DTORS.register(real, state, state);",
"CLOSURE_DTORS.unregister(state)",
)
} else {
("", "")
};

// For shared closures they can be invoked recursively so we
// just immediately pass through `this.a`. If we end up
// executing the destructor, however, we clear out the
Expand All @@ -1986,7 +2007,7 @@ impl<'a> Context<'a> {
self.global(&format!(
"
function makeClosure(arg0, arg1, dtor, f) {{
const state = {{ a: arg0, b: arg1, cnt: 1 }};
const state = {{ a: arg0, b: arg1, cnt: 1, dtor }};
const real = (...args) => {{
// First up with a closure we increment the internal reference
// count. This ensures that the Rust closure environment won't
Expand All @@ -1996,21 +2017,42 @@ impl<'a> Context<'a> {
return f(state.a, state.b, ...args);
}} finally {{
if (--state.cnt === 0) {{
wasm.{}.get(dtor)(state.a, state.b);
wasm.{table}.get(state.dtor)(state.a, state.b);
state.a = 0;
{unregister}
}}
}}
}};
real.original = state;
{register}
return real;
}}
",
table
table = table,
register = register,
unregister = unregister,
));

Ok(())
}

fn expose_closure_finalization(&mut self) -> Result<(), Error> {
if !self.should_write_global("closure_finalization") {
return Ok(());
}
assert!(self.config.weak_refs);
let table = self.export_function_table()?;
self.global(&format!(
"
const CLOSURE_DTORS = new FinalizationRegistry(state => {{
wasm.{}.get(state.dtor)(state.a, state.b)
}});
",
table
));

Ok(())
}
fn global(&mut self, s: &str) {
let s = s.trim();

Expand Down Expand Up @@ -2893,11 +2935,6 @@ impl<'a> Context<'a> {
"false".to_string()
}

Intrinsic::CallbackForget => {
assert_eq!(args.len(), 1);
args[0].clone()
}

Intrinsic::NumberNew => {
assert_eq!(args.len(), 1);
args[0].clone()
Expand Down
2 changes: 2 additions & 0 deletions crates/js-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,7 @@ extern "C" {
/// [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean)
#[wasm_bindgen(constructor)]
#[deprecated(note = "recommended to use `Boolean::from` instead")]
#[allow(deprecated)]
pub fn new(value: &JsValue) -> Boolean;

/// The `valueOf()` method returns the primitive value of a `Boolean` object.
Expand Down Expand Up @@ -1843,6 +1844,7 @@ extern "C" {
/// [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number)
#[wasm_bindgen(constructor)]
#[deprecated(note = "recommended to use `Number::from` instead")]
#[allow(deprecated)]
pub fn new(value: &JsValue) -> Number;

/// The `Number.parseInt()` method parses a string argument and returns an
Expand Down
34 changes: 20 additions & 14 deletions src/closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,22 +356,28 @@ where
}
}

/// Leaks this `Closure` to ensure it remains valid for the duration of the
/// entire program.
/// Release memory management of this closure from Rust to the JS GC.
///
/// > **Note**: this function will leak memory. It should be used sparingly
/// > to ensure the memory leak doesn't affect the program too much.
/// When a `Closure` is dropped it will release the Rust memory and
/// invalidate the associated JS closure, but this isn't always desired.
/// Some callbacks are alive for the entire duration of the program or for a
/// lifetime dynamically managed by the JS GC. This function can be used
/// to drop this `Closure` while keeping the associated JS function still
/// valid.
///
/// When a `Closure` is dropped it will invalidate the associated JS
/// closure, but this isn't always desired. Some callbacks are alive for
/// the entire duration of the program, so this can be used to conveniently
/// leak this instance of `Closure` while performing as much internal
/// cleanup as it can.
pub fn forget(self) {
unsafe {
super::__wbindgen_cb_forget(self.js.idx);
mem::forget(self);
}
/// By default this function will leak memory. This can be dangerous if this
/// function is called many times in an application because the memory leak
/// will overwhelm the page quickly and crash the wasm.
///
/// If the browser, however, supports weak references, then this function
/// 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 :-/

pub fn forget(self) -> JsValue {
let idx = self.js.idx;
mem::forget(self);
JsValue::_new(idx)
}
}

Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,6 @@ externs! {
fn __wbindgen_rethrow(a: u32) -> !;

fn __wbindgen_cb_drop(idx: u32) -> u32;
fn __wbindgen_cb_forget(idx: u32) -> ();

fn __wbindgen_describe(v: u32) -> ();
fn __wbindgen_describe_closure(a: u32, b: u32, c: u32) -> u32;
Expand Down