Use Builtin Atomics for wait/waitAsync/notify

[komyg]

Fix: #899

[aapoalas]

Ok, I've updated the code according to your comment.

Can you take a look if the general idea is good?

Note: it seems that I broke a few tests that were passing, so I still have to fix those.

Yeah, the general idea looks really good to me, excellent stuff! <3

It seems quite likely to me that the reason for the breakage is the too-strict-by-half locking of the mutex, since you're effectively making it so that only one thread can wait or notify concurrently. (I think.) The mutex should only be guarding adding a new waiter and removing an old waiter, but not the waiting itself.

[komyg]

Hey @aapoalas, I've addressed your feedback, can you review again, please?

[aapoalas]

Still some things I'd like changed; most importantly we want to std functions as much as possible.

[aapoalas]

suggestion: Could create a WaiterRecord::new_shared() helper function that returns Arc<WaiterRecord>.

[aapoalas]

issue: I think we should be fine using just Relaxed for the notification. Waking up is our synchronisation, we only really care about the value of the bool here and it is atomic on its own.

[aapoalas]

Oh! I forgot: the ecmascript_futex dependency should be removed.

[aapoalas]

Hmm... I've been looking at the code and doing some cleanups and whatnot and ... I think we have a somewhat fundamental issue here in that the async side has been just bollixed by me. It's pretty badly mangled from what it ought to be now that we're actually using a real parking lot instead of Futexes directly.

Importantly: it seems like the critical section must be entered on the main thread, not on the async thread. eg. the NotEqual check and return you've done on the waiter thread inside the critical section: that's not something that is allowed by the spec, ie. the Promise should never resolve with not_equal. It should be done within the critical section indeed, as you've correctly identified, but if the check fails then we should still return synchronously from the waitAsync function with an object { async: false, value: "not-equal" }.

Maybe we can still hack the effect by changing the signal that we use to wait for the waiter thread to startup to being not a boolean but a state enum: Startup, Started, and NotEqual, but there might be a race condition somewhere there...

If we cannot hack it, we have to actually do the spec-things exactly and create a separate timeout job and a separate handler job. Also we'd need to somehow figure out if we're resolving a promise on our own Agent or not...

[komyg]

Maybe we can still hack the effect by changing the signal that we use to wait for the waiter thread to startup to being not a boolean but a state enum: Startup, Started, and NotEqual, but there might be a race condition somewhere there...

If we cannot hack it, we have to actually do the spec-things exactly and create a separate timeout job and a separate handler job. Also we'd need to somehow figure out if we're resolving a promise on our own Agent or not...

I think we can start with the hack by having the enqueue_atomics_wait_async_job return an Enum with the state, and returning that in the main thread.

By doing that, and addressing your feedback above, I think we will have a solution that is good enough.

Then we can see if it is worth it to take the time to re-write this to be more adherent to the spec.

What do you think?

[aapoalas]

Maybe we can still hack the effect by changing the signal that we use to wait for the waiter thread to startup to being not a boolean but a state enum: Startup, Started, and NotEqual, but there might be a race condition somewhere there...
If we cannot hack it, we have to actually do the spec-things exactly and create a separate timeout job and a separate handler job. Also we'd need to somehow figure out if we're resolving a promise on our own Agent or not...

I think we can start with the hack by having the enqueue_atomics_wait_async_job return an Enum with the state, and returning that in the main thread.

By doing that, and addressing your feedback above, I think we will have a solution that is good enough.

Then we can see if it is worth it to take the time to re-write this to be more adherent to the spec.

What do you think?

Yeah, I guess that could work. I have something akin to this (without the enum) working locally on my laptop with a lot of cleanups, and I was just running test262 on it when the laptop ran out of battery and it turns out I've left my charger at work!

Based on my experience there, I must admit I'm not exactly thrilled about having the critical section effectively entirely on a separate thread with the main thread having to wait for a sync message from the waiter thread to see if the waiter thread's read of the byte index produced a "notEqual" result or not... But I think we can live with it temporarily.

Looking at it from a performance point of view now that we have a proper parking lot futex instead of native OS futexes, it makes sense that we'll avoid spawning a waiter thread unless specifically needed for a timeout (and actually even this should be delegated to the host, so that the host can decide if a timeout thread is needed or if it should instead be some sort of OS timer API that we rely on). For waitAsync in particular, the user expectation is that they'll wait for a sync message to come in from a foreign thread but while they wait for that, they want to continue working on other things. Spawning a thread is an unnecessarily complex thing to do when they could just check an atomic boolean after they've done the "other things" to see if a sync message has arrived or not.