Mark asyncify exports

[RReverser]

Names of all the asyncified export functions are now added as data of custom "asyncify" sections.

This allows to detect them in a JS wrapper and wrap into async JS functions only if necessary.

Please check commit descriptions for implementation details about what's considered an asyncified function.

Note that this PR currently also includes #2326, because it depends on it for testing, and #2324 because otherwise I was getting failures on Windows with CMake + Ninja. When these are merged, I can rebase this one.

Fixes #2322.

[kripken]

Nice, in general this looks great!

I'm not sure how I feel about a separate custom section for each function name. It seems like a single section with a list would be more consistent with how other tool convention sections currently work?

[RReverser]

I'm not sure how I feel about a separate custom section for each function name. It seems like a single section with a list would be more consistent with how other tool convention sections currently work?

Yeah, I described this in the relevant commit description:

We could come up with a more condensed representation that doesn't duplicate export function names or name of "asyncify" section itself, but this would add unnecessary complexity to implementation with little benefit after Gzip / Brotli, which already take care of duplicated strings quite well.

I'm still not sure about it myself, but it feels easier to maintain this way since it allows supporting arbitrary characters in export names without employing some escaped format (like JSON).

[tlively]

I think the concern here is consistency of design, not compactness of representation. You can support arbitrarily many names in one section using the standard vector encoding and support arbitrary characters in names by using the standard name encoding. Using these encodings in one custom section rather than having a large number of small custom sections that don't use the standard name encoding would be more similar to how other custom sections are laid out and used.

[RReverser]

@tlively It's certainly possible, but requires quite a bit more logic and code on the JavaScript side (the main consumer of this section) to decode both encodings back, whereas multiple same-named custom sections are already allowed and decoding is as easy as WebAssembly.customSections(m, 'asyncify').map(b => textDecoder.decode(b)).

[kripken]

Those are both good points, I'm not sure what's best here. It is indeed simpler from JS, but it's less consistent with the others... Maybe worth discussing on tool-conventions with more people?

[hostilefork]

Names of all the asyncified export functions are now added as data of custom "asyncify" sections.
This allows to detect them in a JS wrapper and wrap into async JS functions only if necessary.

Maybe the behavior is more conservative and warning-giving than this description says...but doing this automatically this sounds like it could create some confusion if it were a default. Asyncification appears semi-automatic based on transitive closure decisions...and something that could get smarter over time as more clever optimizations are figured out. Hence someone could make a minor change in the C codebase, leading to a change to a Promise or to not-a-Promise in an API. Or am I misunderstanding how this detection is proposed to work?

OTOH: I think that having the knowledge of whether something needs {async: true} could inform asserts, e.g. the ones which are being removed with some reservations here:

emscripten-core/emscripten#9423

So perhaps the automatic part is just automatically warning you in the asserts build when you're missing the {async: true} that you need?

Please check commit descriptions for implementation details about what's considered an asyncified function.

When I posted on the newsgroup that I'd like to see this list, @kripken pointed me to this PR. It would be great to have an easy mode to list out the functions that were and weren't asyncified to do a check against intuition.

(I'm not clued in on exactly how much data about the call graph one has--but ideally if you blacklist a function, and it calls other functions known to be called exactly once those will get blacklisted too?)

Though I did do a "dumb" test to just blacklist functions I thought should be blacklisted, and noticed the wasm getting smaller when I did. Some of these functions are inline--which I imagine creates problems because the parts that do get inlined are beholden to the asyncify status of the function that calls them. (?) If the asyncifying was at the basic block level that would presumably help.

(I probably shouldn't complain about these things unless I want to step up and write it, eh? :-P)

[RReverser]

@hostilefork I think this is a valid potential concern, but I see this change mostly as a way to opt-out from async code when you know that function is definitely synchronous.

That is, you can still use await ... for majority of functions, since it works with both regular and Promise<...> values, but 1) it will be faster for functions that don't actually need rewinding/unwinding wrapper and 2) you get ability to call functions that you know are synchronous, as synchronous, whereas currently you don't have such choice.

[hostilefork]

@hostilefork I think this is a valid potential concern, but I see this change mostly as a way to opt-out from async code when you know that function is definitely synchronous.

To me, the idea of something returning a promise requires it to be named very specifically telling you so in the interface contract. e.g. I don't see myself throwing await on routines just because they might be asynchronous. I want the API documentation for what I'm using to commit one way or another--and introduce a differently-named entry point if there's a change.

I guess if people are convinced that everything cwrap()'d must be await'ed on unless specified otherwise, that is the way it goes. But it seems a pretty major change--and one that runs counter to my personal preferences...

[RReverser]

@hostilefork I think there is misunderstanding in what this PR does. You (or, rather, JS wrapper) are still in control of the actual API and can provide own names or wrap everything into async functions etc.

This just provides a metadata so that JS would know when function doesn't actually need wrapping and state management, but rather can be called directly as a regular synchronous function.

[hostilefork]

This just provides a metadata so that JS would know when function doesn't actually need wrapping and state management, but rather can be called directly as a regular synchronous function.

If it doesn't change the current status quo:

1. no functions get today's behavior of {async: true} unless you ask using cwrap
2. a function not annotated {async: true} which tries to yield causes an error

Then no problem. It was just the wording of "Names of all the asyncified export functions are now added as data of custom "asyncify" sections. This allows to detect them in a JS wrapper and wrap into async JS functions only if necessary." which made me concerned the {async: ...} flag was being automatically chosen.