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

Wrapped exports should return Promises #11

Closed
fgmccabe opened this issue Aug 29, 2022 · 18 comments
Closed

Wrapped exports should return Promises #11

fgmccabe opened this issue Aug 29, 2022 · 18 comments

Comments

@fgmccabe
Copy link
Collaborator

The current spec of JSPI returns a Promise if the underlying wasm suspended (due to a call to a suspending import) and returns a regular value if the export 'returned normally'.
This is not a recommended approach to designing API for the web: return types of the form JSPromise | AnythingNotAPromise are not recommended.
The fix is to always return a Promise, and to have the final return value be the equivalent of
Promise.resolve(innerExport())

@dschuff
Copy link
Member

dschuff commented Sep 1, 2022

Does this mean that it will always be impossible to avoid a trip through the microtask queue every time a wrapped export is called?

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Sep 1, 2022

Essentially, yes. Calling a wrapped export will inevitably result in that trip.
On the flip side, at least it will be uniform: when you call a wrapped export, you will get a promise. Which means that you will not need to bifurcate your business logic.

@thibaudmichaud
Copy link
Contributor

Suspending imports can also return a JSPromise | AnythingNotAPromise. Should it also wrap non-Promise results in a Promise.resolve()?

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Sep 7, 2022

Is this forced on us?

@tlively
Copy link
Member

tlively commented Sep 13, 2022

Atomics.waitAsync bends over backwards to avoid the extra trip through the microtask queue. It returns an object that contains a bool signifying whether the other member is a promise or raw value: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/waitAsync#return_value. If we want to avoid the trip through the microtask queue, I assume we would need to do something similar. It may not be worth it.

@RossTate
Copy link
Collaborator

Cool example! My sense is that their design to avoid the microtask queue is more heavy-handed than what people would want here, so my inclination at the moment would be that "consistently require and produce promises" is the right default here, but I'm definitely open to the alternative if people would prefer it.

@rektide
Copy link

rektide commented Sep 27, 2022

I agree fully that this is accepted dogmatism for the web. It felt like something that emerged very sharp & very hard right as promises were being standardized, & it doesn't feel like there's been a lot of discussion or updates now that promises are here. I wish there were a way to reopen this discussion better, because it feels like there is so much performance we leave on the table for something that rarely has been a problem for developers.

With tools like await there is now far less impact to returning either a value or a promise, where-as historically handling value-or-promise required considerable care. The hazard here has been largely ameliorated.

Personally I'd love to see the spec embrace the faster route. Let higher-level tools wrap & alter their wasm exports as they'd like to take the hit in the name of (now not so relevant) convenience, but leave the low level capability to be fast possible!

@tlively
Copy link
Member

tlively commented Sep 28, 2022

AFAIU, await doesn't solve the problem because awaiting a non-promise value implicitly creates a promise and returns to the microtask queue anyway. We would need something like fast_await that continues directly if the awaited object is not a promise.

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Oct 3, 2022

IMO, the performance argument does not hold against the destruction of code integrity.
If an export has been marked as promising, that means that the application has to be constructed in such a way as to anticipate that it did return a promise. Allowing the export to not actually return a promise complicates the logic of the application.
OTOH, for unpromising (sic) exports, there is no such expectation; and they can continue to be as fast as ever.

@tlively
Copy link
Member

tlively commented Oct 3, 2022

I don't think we should rule out the possibility that the performance problems might be bad enough that we will need to do something about them.

For instance, in the wasm-split use case, nearly all the exports will need to be wrapped and JS will have to be prepared for them to return a promise in case they trigger the loading of a secondary module and suspend. However, on each thread, there will only ever be a single suspension per secondary module, and after the secondary modules have been loaded, the program will never suspend again (all assuming the program does not otherwise use JSPI).

For that use case, forcing the JS to return to the event loop on every call to an export is pure overhead in the limit. An alternative solution would be to have both a wrapped and unwrapped version of each export and have a macro to choose between them based on whether the secondary module has been loaded, and perhaps that's enough, but is that what we really want producers to have to do?

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Oct 3, 2022

This does not address the code question. If you call a function that might return a Promise, you have to code it in such a way that it will. Instead of two exports, you force the applications programmer to have two call paths to the code.

@tlively
Copy link
Member

tlively commented Oct 3, 2022

Right, to avoid wait overhead there would always have to be two code paths: one that awaits a promise and one that does not. Note that these code paths could be encapsulated in a macro or similar. This is true whether the two code paths call different versions of the export or whether they call the wrapped export and inspect its result. The difference is that the former solution requires being able to tell in advance whether an export may suspend, which is possible in limited wasm-split use cases but not in general.

Would it be possible to have the program configure the wrapper to either always return a promise or to return an object like the one returned by Atomics.waitAsync? (The default could be to always return a promise) That way programs could opt in to handling this additional complexity where the performance benefit is worth it.

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Oct 3, 2022

That is an interesting suggestion. Worth considering.
I do however, pushback on the performance question. If a wasm module is big enough to warrant code splitting, I find it unlikely that it will be so sensitive to the performance of an individual export like this.

@ajklein
Copy link
Collaborator

ajklein commented Oct 3, 2022

Waiting on an eagerly-resolved Promise does not return to the event loop, only the microtask queue (which runs, effectively, when the JS stack unwinds but before returning control to the event loop). So the performance should be strictly better than returning to the event loop, at least.

@tlively, what would this "macro" look like from the JS point of view? It's hard for me to imagine JS code that transparently switches between sync and async.

@tlively
Copy link
Member

tlively commented Oct 4, 2022

Yeah, I admit that there might not actually be a performance problem here and that ideally we would get some data to inform this choice. I'm just worried about this becoming one of those "peanut butter" performance problems where it's spread all over the system making everything just a little slower.

I don't know about any kind of macro system in use in normal Web dev, but in Emscripten we literally run (a reimplementation of) the C preprocessor over our JS and also have some AST-rewriting passes to expand macros that look like normal function calls, so it would be totally trivial to implement such a macro.

Specifically, with the more limited, duplicated-exports solution for wasm-split, the macro would expand to this:

if (secondaryModuleLoaded) {
  x = unwrappedExport();
} else {
  x = await wrappedExport();
}

And with the solution where the wrapped function may or may not return a promise:

x = wrappedExport();
if (looksLikePromise(x)) {
  x = await x;
}

Of course then you run into the issue where the unwrapped, raw return value x also looks like a promise, and that's where the return scheme of Atomics.waitAsync can help disambiguate.

@ajklein
Copy link
Collaborator

ajklein commented Oct 4, 2022

I'm fairly sure CPP is not common in the JS community, and the assumption here that you're inside an async function may not always hold either. I'll also point out that the W3C TAG guidelines on using Promise in web specs suggests unconditionally returning promises.

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Oct 4, 2022

This is good to know, especially since I agree with this :)

@fgmccabe
Copy link
Collaborator Author

fgmccabe commented Oct 6, 2022

After some off-line discussions, this is how we should proceed:

  1. The MAP version of the API will always return a promise. This simplifies the caller logic as well as simplifying internal handling of exceptions. Essentially, a wrapped export returns a promise: the promise will eventually be resolved with the value returned by the export; or, in the case that the export raises and exception or traps, the promise's reject function will be called.
  2. In a future version of the API, depending on feedback from the user community, we will extend the API to allow an application developer to specify wether the wrapped export should always return a Promise or only do so when suspending (at least once).

@fgmccabe fgmccabe closed this as completed Oct 6, 2022
pull bot pushed a commit to jamlee-t/v8 that referenced this issue Apr 28, 2023
The export wrapper should always return a promise, even if it does not
suspend, and even if an exception is thrown. Context:
WebAssembly/js-promise-integration#11

This is addressed by creating a promise in a pending state,
and explicitly fulfilling it / rejecting it as needed:
- Assign a unique pending promise to each Suspender object,
- When the export returns, fulfill the promise with the return value,
  and return it,
- When the export throws, detect the stack-switch frame in the unwinder,
  reject the promise, and return normally from the export,
- When the export suspends, return the (pending) promise. Attach the
  resume/reject callbacks to the promise returned by the import, but
  discard the result

In addition to meeting the requirements of the new API, this has a
significant performance benefit. By manually fulfilling or rejecting
this promise, we don't need to keep all the intermediate promises alive.
In the previous implementation, these promises were implicitly chained
and kept alive by the GC, causing a significant overhead linear with the
number of suspensions/resumptions.

R=ahaas@chromium.org

Bug: v8:12191
Change-Id: I2a26735440dab711f4ae767b32bdc4f72acad112
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4467027
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87335}
pull bot pushed a commit to jamlee-t/v8 that referenced this issue Apr 28, 2023
This reverts commit 4d7d717.

Reason for revert: Fails on clusterfuzz arm64 mksnapshot build:
https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Clusterfuzz%20Linux64%20ASAN%20arm64%20-%20debug%20builder/34987/overview

Original change's description:
> [wasm] Update the JSPI API
>
> The export wrapper should always return a promise, even if it does not
> suspend, and even if an exception is thrown. Context:
> WebAssembly/js-promise-integration#11
>
> This is addressed by creating a promise in a pending state,
> and explicitly fulfilling it / rejecting it as needed:
> - Assign a unique pending promise to each Suspender object,
> - When the export returns, fulfill the promise with the return value,
>   and return it,
> - When the export throws, detect the stack-switch frame in the unwinder,
>   reject the promise, and return normally from the export,
> - When the export suspends, return the (pending) promise. Attach the
>   resume/reject callbacks to the promise returned by the import, but
>   discard the result
>
> In addition to meeting the requirements of the new API, this has a
> significant performance benefit. By manually fulfilling or rejecting
> this promise, we don't need to keep all the intermediate promises alive.
> In the previous implementation, these promises were implicitly chained
> and kept alive by the GC, causing a significant overhead linear with the
> number of suspensions/resumptions.
>
> R=​ahaas@chromium.org
>
> Bug: v8:12191
> Change-Id: I2a26735440dab711f4ae767b32bdc4f72acad112
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4467027
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#87335}

Bug: v8:12191
Change-Id: Ia7e20009dc206fc888ba203a662cb9a7837a041f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4487895
Commit-Queue: Matthias Liedtke <mliedtke@chromium.org>
Owners-Override: Matthias Liedtke <mliedtke@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#87337}
pull bot pushed a commit to jamlee-t/v8 that referenced this issue May 18, 2023
This is a reland of commit 4d7d717

Original change's description:
> [wasm] Update the JSPI API
>
> The export wrapper should always return a promise, even if it does not
> suspend, and even if an exception is thrown. Context:
> WebAssembly/js-promise-integration#11
>
> This is addressed by creating a promise in a pending state,
> and explicitly fulfilling it / rejecting it as needed:
> - Assign a unique pending promise to each Suspender object,
> - When the export returns, fulfill the promise with the return value,
>   and return it,
> - When the export throws, detect the stack-switch frame in the unwinder,
>   reject the promise, and return normally from the export,
> - When the export suspends, return the (pending) promise. Attach the
>   resume/reject callbacks to the promise returned by the import, but
>   discard the result
>
> In addition to meeting the requirements of the new API, this has a
> significant performance benefit. By manually fulfilling or rejecting
> this promise, we don't need to keep all the intermediate promises alive.
> In the previous implementation, these promises were implicitly chained
> and kept alive by the GC, causing a significant overhead linear with the
> number of suspensions/resumptions.
>
> R=ahaas@chromium.org
>
> Bug: v8:12191
> Change-Id: I2a26735440dab711f4ae767b32bdc4f72acad112
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4467027
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#87335}

Bug: v8:12191
Change-Id: I33370eb51894c6bba1b2b865c5715bb75b71d64c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4536275
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87736}
pull bot pushed a commit to jamlee-t/v8 that referenced this issue May 18, 2023
This reverts commit dbc899c.

Reason for revert: breaks asan

Original change's description:
> Reland "[wasm] Update the JSPI API"
>
> This is a reland of commit 4d7d717
>
> Original change's description:
> > [wasm] Update the JSPI API
> >
> > The export wrapper should always return a promise, even if it does not
> > suspend, and even if an exception is thrown. Context:
> > WebAssembly/js-promise-integration#11
> >
> > This is addressed by creating a promise in a pending state,
> > and explicitly fulfilling it / rejecting it as needed:
> > - Assign a unique pending promise to each Suspender object,
> > - When the export returns, fulfill the promise with the return value,
> >   and return it,
> > - When the export throws, detect the stack-switch frame in the unwinder,
> >   reject the promise, and return normally from the export,
> > - When the export suspends, return the (pending) promise. Attach the
> >   resume/reject callbacks to the promise returned by the import, but
> >   discard the result
> >
> > In addition to meeting the requirements of the new API, this has a
> > significant performance benefit. By manually fulfilling or rejecting
> > this promise, we don't need to keep all the intermediate promises alive.
> > In the previous implementation, these promises were implicitly chained
> > and kept alive by the GC, causing a significant overhead linear with the
> > number of suspensions/resumptions.
> >
> > R=ahaas@chromium.org
> >
> > Bug: v8:12191
> > Change-Id: I2a26735440dab711f4ae767b32bdc4f72acad112
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4467027
> > Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> > Reviewed-by: Andreas Haas <ahaas@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#87335}
>
> Bug: v8:12191
> Change-Id: I33370eb51894c6bba1b2b865c5715bb75b71d64c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4536275
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#87736}

Bug: v8:12191
Change-Id: I92f339fbd5a766642b26e08ca589f82f3d2dc2cb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4545080
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Thibaud Michaud <thibaudm@chromium.org>
Owners-Override: Matthias Liedtke <mliedtke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87737}
pull bot pushed a commit to jamlee-t/v8 that referenced this issue May 24, 2023
This is a reland of commit dbc899c

Original change's description:
> Reland "[wasm] Update the JSPI API"
>
> This is a reland of commit 4d7d717
>
> Original change's description:
> > [wasm] Update the JSPI API
> >
> > The export wrapper should always return a promise, even if it does not
> > suspend, and even if an exception is thrown. Context:
> > WebAssembly/js-promise-integration#11
> >
> > This is addressed by creating a promise in a pending state,
> > and explicitly fulfilling it / rejecting it as needed:
> > - Assign a unique pending promise to each Suspender object,
> > - When the export returns, fulfill the promise with the return value,
> >   and return it,
> > - When the export throws, detect the stack-switch frame in the unwinder,
> >   reject the promise, and return normally from the export,
> > - When the export suspends, return the (pending) promise. Attach the
> >   resume/reject callbacks to the promise returned by the import, but
> >   discard the result
> >
> > In addition to meeting the requirements of the new API, this has a
> > significant performance benefit. By manually fulfilling or rejecting
> > this promise, we don't need to keep all the intermediate promises alive.
> > In the previous implementation, these promises were implicitly chained
> > and kept alive by the GC, causing a significant overhead linear with the
> > number of suspensions/resumptions.
> >
> > R=ahaas@chromium.org
> >
> > Bug: v8:12191
> > Change-Id: I2a26735440dab711f4ae767b32bdc4f72acad112
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4467027
> > Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> > Reviewed-by: Andreas Haas <ahaas@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#87335}
>
> Bug: v8:12191
> Change-Id: I33370eb51894c6bba1b2b865c5715bb75b71d64c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4536275
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#87736}

Bug: v8:12191
Change-Id: Ie0f7f5bf7ec2fb8edb4227ed0e89e598f80fe222
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4545082
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87824}
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

No branches or pull requests

7 participants