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

Expose asyncify state via a getter #2679

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

RReverser
Copy link
Member

Normally, a wrapper has to track state separately to know when to unwind/rewind and when to actually call import functions.

Exposing Asyncify state can help avoid this duplication and avoid subtle bugs when internal and wrapper state get out of sync.

Since this is a tiny function and it's useful for any Asyncify embedder, I've decided to expose it by default rather than hide behind an option.

Normally, a wrapper has to track state separately to know when to unwind/rewind and when to actually call import functions.

Exposing Asyncify state can help avoid this duplication and avoid subtle bugs when internal and wrapper state get out of sync.

Since this is a tiny function and it's useful for any Asyncify embedder, I've decided to expose it by default rather than hide behind an option.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Sounds good. Adds a little code but in tools that can be optimized out if not used (like metadce in emscripten).

Lines 130-150 have some doc description of the exported functions, please add to there.

@RReverser
Copy link
Member Author

Adds a little code but in tools that can be optimized out if not used (like metadce in emscripten).

I think it will also help remove some code on the JS side and might even result in a net negative. But even if not, it's a negligible difference compared to general Asyncify overhead anyway :)

Lines 130-150 have some doc description of the exported functions, please add to there.

Will do!

@RReverser
Copy link
Member Author

Lines 130-150 have some doc description of the exported functions, please add to there.

Done.

@kripken
Copy link
Member

kripken commented Mar 4, 2020

I think it will also help remove some code on the JS side

True, but it would mean a JS=>wasm call instead of a JS property lookup, which would be slower. In emscripten I think probably we wouldn't use it (but definitely sounds useful for other places!).

@kripken
Copy link
Member

kripken commented Mar 4, 2020

Oh, looks like line 130 has "four" which is no longer true after this change ;) please update that (can commit with [ci skip] to not wait, and avoid CI time.

@RReverser
Copy link
Member Author

Thanks, updated. Looks like CI is still waiting at least to get started though.

@kripken
Copy link
Member

kripken commented Mar 4, 2020

Thanks!

@kripken kripken merged commit 57a81a0 into WebAssembly:master Mar 4, 2020
@RReverser RReverser deleted the expose-asyncify-state branch March 4, 2020 22:32
RReverser added a commit to GoogleChromeLabs/asyncify that referenced this pull request Apr 23, 2021
 - Use asyncify_get_state now exported by Asyncify transform (see WebAssembly/binaryen#2679).
 - Improve error messages for missing import objects (previously would throw obscure Proxy construction error).
mrbbot added a commit to mrbbot/wasm-pack that referenced this pull request Jul 18, 2021
- This version exports `asyncify_get_state` as required by GoogleChromeLabs/asyncify (see WebAssembly/binaryen#2679)
- This version also uses the same artifact names, so minimal code changes are required
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