wip make select() async when using ASYNCIFY/JSPI

[inolen]

Hey, this isn't actually ready for merge - I just wanted to get a discussion going to figure out the correct way to implement this.

The quake3 engine uses select() with a timeout to throttle the server frame rate, while still allowing it to process network messages between frames. Using emscripten_set_main_loop doesn't work great here, as the server a.) is limited to handling networking at frame boundaries and b.) the timing isn't very consistent.

This PR doesn't contain an actual async poll implementation for the WebSocket backend - I added that on my wip WebTransport backend, which I'll post a separate PR for soon. However, the problem is still how to correctly handle making the syscall (and then everything else down the callstack that does async work) conditionally async. Sprinkling around #if ASYNCIFY and __async didn't feel great, but it did work.

[sbc100]

Nice idea! Its interesting that we just added potential for select to block in #25523, but I assume you two are working independently?

I have some ideas knocking around about how I think this stuff could be made to be blocking also when called from threads: #25970

I'd also like to refactor such that this version of select can be deleted and we can implement just poll instead (because select can be implemented in native code in terms of poll).

But I like this idea!

[inolen]

Does the preprocessor support some way to conditionally add the async keyword in front of a function? For example, could I do like -

{{{ ASYNC }}} () => {
...
}

and also then

{{{ AWAIT }}} poll();

Where these are empty when ASYNCIFY is disabled?

That would at least make the #if ASYNCIFY stuff a bit nicer, but it wouldn't ensure that the __async decorator is set still.

[sbc100]

Yes, we already have something like that. See {{{ asyncIf(WASM_ASYNC_COMPILATION) }}}

[inolen]

Yes, we already have something like that. See {{{ asyncIf(WASM_ASYNC_COMPILATION) }}}

Is there anything also dealing with the decorator? I was trying to think if there was some way to generate a structure and abuse the spread operator to make that auto-magic.

The idea being to end up pasting something like -

...{
func__async: true,
func: async () => {
...
}
)

but doing that by pasting in something up front would require the rogue extra } at the end. Can the preprocessor take in a JS function body as an argument somehow?

[sbc100]

See the existing uses of asyncIf for example, dlopen_js

[sbc100]

I also happen to be working on select/poll and blocking myself right now. See #25985 which is a followup to #25523

[inolen]

dlopen_js

Right, that's what I was trying to noodle to see if it was possible to improve.

#if ASYNCIFY
  _dlopen_js__async: true,
#endif
  _dlopen_js: {{{ asyncIf(ASYNCIFY == 2) }}} (handle) =>

That's effectively what I'm doing here, but I was trying to think if there was a better way to do that such that both the decorator and async prefix were emitted as a result.

For example, I wonder if it's possible to do like -

{{{
ASYNC_IF(ASYNCIFY == 2, myfunc, () => {
... my function body
})
}}}

and that emit out

...{
myfunc__async: true,
myfunc: async () => {}
}

[inolen]

I also happen to be working on select/poll and blocking myself right now. See #25985 which is a followup to #25523

Sure, not trying to step on toes here, just trying to find a way to have a way for select() to end up being async (when single threaded).

[sbc100]

I also happen to be working on select/poll and blocking myself right now. See #25985 which is a followup to #25523

Sure, not trying to step on toes here, just trying to find a way to have a way for select() to end up being async (when single threaded).

Absolutely, I think we would be great if we could have single approach that would work for both singlethread-JSPI/ASYNCIFY and proxied-from-background thread. Thats what #25970 is all about.

Any help towards that goal is much appreciated!

[sbc100]

BTW, we don't have to actually mark a function as async for it to work to actually return a promise (i.e. work with JSPI). We can have non-async functions that return promises sometimes and non-promises other times. e.g. select and poll might return a promise when called on the main thread but be blocking when called on a background thread and return synchronously.

Marking a functions as async means it can never return synchronously and therefore is not useful when JSPI is disabled so we might have reasons to use the async tag even when we return promises.

[inolen]

BTW, we don't have to actually mark a function as async for it to work to actually return a promise (i.e. work with JSPI). We can have non-async functions that return promises sometimes and non-promises other times. e.g. select and poll might return a promise when called on the main thread but be blocking when called on a background thread and return synchronously.

Marking a functions as async means it can never return synchronously and therefore is not useful when JSPI is disabled so we might have reasons to use the async tag even when we return promises.

Hmm true, we only need the async keyword to then use await (i think), but this is all just syntactic sugar at the end of the day.

[inolen]

@sbc100 these changes were circling around in my head last night while checking out the updated code after reading some of the current issues / pr's regarding poll. I copied what I did on my WebTransport code over to the WebSocket backend here.

The main changes -

• split up sync / async code inside __syscall_poll
• made timeout < 0 set timeout = 0x7fffffff to cut down on timeout special cases
• removed callback cleanup logic. given the wake up callback is dynamically generated and has the per-poll awakened flag, it's effectively removed once that's set
• altered logic added by @ktock regarding the callback passed to streams - the callback was changed to pass in an event mask so poll can decide if it needs to do something with the wake up. poll then no longer immediately resolves, but repolls on wake up. this replaces the assert(flags) inside of makeNotifyCallback with that guarantee at runtime and provides the opportunity for streams to wake up for different events separately (e.g. read and write). it also gives an opportunity for the runtime environment to update multiple streams back to back before repolling (which is wrapped in a setTimeout(0) on wake up)
• @ktock's changes had already started this, but it's no longer really async polling, as much as it is repolling in async contexts on wake up from a stream. it's optional for streams to implement, with the gotcha being that when streams don't implement it, the max timeout will be waited on if nothing is immediately available in async contexts. could add a few helpers for registering / removing the callback from a stream's wake queue, and the poll() implementation could avoid calling setTimeout if the wake callback isn't ever registered (if none are registered, nothing will ever wake it up).

[sbc100]

Would you mind splitting out the renaming here, so this change becomes more focused?

[sbc100]

Or just skip it since snake seems to be used a fair amount in the FS layer in general?

[inolen]

No problem skipping that, I just couldn't decide which variable to match up with since it's a mixed bag. I'll go with wake_queue.

[sbc100]

I'm not sure I love the timeout = 0x7fffffff; thing above.. and one downside is that it means we end up setting this timeout here even for the "infinite timeout" case.

i.e. i think it would be nice to skip this timeout completely in that case.

Does timeout = 0x7fffffff; really make things easier in other places?

[inolen]

Looking at the code, it doesn't matter now. I had added that while shifting the code around trying to get the async code into the single #ifdef / Promise constructor, but didn't pay attention to its value after the dust settled.

[sbc100]

Is this return value used somwhere?

What does it mean to return false here? Does it mean we got some kind of spurious wakeup? I.e. the wakeup was triggered with the wrong set of events? Could we not ban this? i.e. should the caller contract be that we only call the wake callback once you have one of the events we care about?

[inolen]

Preface - this is ultimately just a dumbed down design stolen from the linux kernel's poll_wait design and I'm not married to the details. So yes returning false would mean it's a spurious wake up - the stream maybe woke it up due to a POLLIN but poll only cared about POLLOUT.

I think it's fine to make the stream op take in poll(stream, events, wake) and do as you said. Then there's no return value and the stream is always removing any wake callback who's event mask matches whatever event that's occurred.

[inolen]

Also regarding the concern for streams not implementing the wake support - instead of passing events and wake, you could make that a callback that returns { events, wake }. If it's not called, no timeouts are ever set.