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

Move closure shims into the descriptor #1065

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

alexcrichton
Copy link
Contributor

Currently closure shims are communicated to JS at runtime, although at
runtime the same constant value is always passed to JS! More pressing,
however, work in #1002 requires knowledge of closure descriptor indices
at wasm-bindgen time which is not currently known.

Since the closure descriptor shims and such are already constant values,
this commit moves the descriptor function indices into the descriptor
for a closure/function pointer. This way we can learn about these values
at wasm-bindgen time instead of only knowing them at runtime.

This should have no semantic change on users of wasm-bindgen, although
some closure invocations may be slightly speedier because there's less
arguments being transferred over the boundary. Overall though this will
help #1002 as the closure shims that the Rust compiler generates may not
be the exact ones we hand out to JS, but rather wrappers around them
which do anyref business things.

@alexcrichton alexcrichton requested a review from fitzgen November 29, 2018 20:19
Currently closure shims are communicated to JS at runtime, although at
runtime the same constant value is always passed to JS! More pressing,
however, work in rustwasm#1002 requires knowledge of closure descriptor indices
at `wasm-bindgen` time which is not currently known.

Since the closure descriptor shims and such are already constant values,
this commit moves the descriptor function indices into the *descriptor*
for a closure/function pointer. This way we can learn about these values
at `wasm-bindgen` time instead of only knowing them at runtime.

This should have no semantic change on users of `wasm-bindgen`, although
some closure invocations may be slightly speedier because there's less
arguments being transferred over the boundary. Overall though this will
help rustwasm#1002 as the closure shims that the Rust compiler generates may not
be the exact ones we hand out to JS, but rather wrappers around them
which do `anyref` business things.
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Nice! more static = more good

} else {
// ... otherwise we're injecting a number of new imports, so offset
// everything.
idx + info.code_idx_to_descriptor.len() as u32
Copy link
Member

Choose a reason for hiding this comment

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

This particular change is just code motion / reformatting, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes indeed!

@alexcrichton alexcrichton merged commit 91e9495 into rustwasm:master Nov 29, 2018
@alexcrichton alexcrichton deleted the describe-closures branch November 29, 2018 23:31
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.

2 participants