feat: support Atomics.waitAsync #687
Conversation
metcoder95
commented
Oct 30, 2024
•
metcoder95
commented
- feat: Support Atomics.waitAsync
Co-authored-by: Robert Nagy <[email protected]>
… 6.12.0 (#453) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
Adding test-case is pending |
src/worker.ts
Outdated
| // @ts-expect-error | ||
| const { async, value } = Atomics.waitAsync(sharedBuffer, kRequestCountField, lastSeenRequestCount); | ||
|
|
||
| return async === true && value; |
There was a problem hiding this comment.
They're a IIFE that has a catch to handle the errors; it is not awaited explicitly to avoid wrapping a non-promise into a Promise, nevertheless we might be loosing some stack out of the stacktrace. although as it is executed in a single place it might not be that bad but happy to hear your thoughts
There was a problem hiding this comment.
I still don't see how it's awaited... Maybe to complicated for me to understand
There was a problem hiding this comment.
Maybe I didn't get what you meant at the first time, but think I might get it now. At a top-level is not really awaited, but more on a fire-and-forget approach, so the Promise is not awaited for resolution but triggered and any error is handled.
This happens on every new message or at a startup
There was a problem hiding this comment.
That doesn't sound good? Though I don't know enough of the current design to truly know.
There was a problem hiding this comment.
Yeah, while writing this, I noticed the same; let me see if there's any side-effect on awaiting on it.
I don't see a potential problem as anyways we already await for the task handler to resolve the task, so it should come for free; but let me check and benchmark
There was a problem hiding this comment.
Refactored into awaiting for the wake up; although the benchmarks requires some re-work, the refactor reflected a nice performance boost
metcoder95
force-pushed
the
feat/async_atomics
branch
from
4b660db
to
ba71411
Compare
| returning control to the main thread (avoid having open handles within a thread). If still want to have the possibility | ||
| of having open handles or handle asynchrnous tasks, you can set the environment variable `PISCINA_ENABLE_ASYNC_ATOMICS` to `1` or setting `options.atomics` to `async`. | ||
|
|
||
| > **Note**: The `async` mode comes with performance penalties and can lead to undesired behaviour if open handles are not tracked correctly. |
There was a problem hiding this comment.
Can you explain the open handle thing? Why is that an issue with async?
There was a problem hiding this comment.
Mostly as we are not doing event loop ticks, these can add some overhead if the workers start doing more than it should.
Even waiting for just the worker to wake up added some ticks that showed a difference of ±5% which is really negligible.
I've not strong opinion in this one; but if we want to make this the default behaviour we should document that it is important to not keep the workers doing too much background tasks as they can face some penalty (as well if using async, implementer needs to account for the teardown of the worker when idle, i.e. no more tasks scheduled)
There was a problem hiding this comment.
If that sounds good, I can adjust the PR for that 👍
|
I would recommend making async the default for a future semver major. IMHO it's the best balance between perf and expected behavior. |
