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

Atomic condition variable #5

Open
ronag opened this issue Nov 14, 2022 · 6 comments
Open

Atomic condition variable #5

ronag opened this issue Nov 14, 2022 · 6 comments
Labels
benchmark-needed Add to issues that does not have a benchmark

Comments

@ronag
Copy link
Member

ronag commented Nov 14, 2022

I've noticed that using Atomics.notify + Atomics.waitAsync is extremely slow. Just doing a normal Atomics.load + setTimeout polling loop is a lot faster.

Since Atomics is implemented in V8 I'm not sure there is much we can do about it. Other than maybe adding notes in our docs.

Refs: https://github.com/pinojs/thread-stream/blob/main/lib/wait.js
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13483

@ronag
Copy link
Member Author

ronag commented Nov 14, 2022

@jasnell @addaleax Might be relevant for piscina?

@ronag
Copy link
Member Author

ronag commented Nov 14, 2022

From a quick look in the V8 source code it looks notify is implemented verbatim with the spec without any optimisations, i.e. there is no fast path such as just checking the stored value, and it also uses a global mutex, every time.

@anonrig
Copy link
Member

anonrig commented Nov 14, 2022

Can you provide a micro-benchmark?

@ronag
Copy link
Member Author

ronag commented Nov 14, 2022

Not really... what would I compare it with?

@anonrig
Copy link
Member

anonrig commented Nov 14, 2022

I assume it is: Atomics.notify + Atomics.waitAsync vs Atomics.load + setTimeout

@ronag
Copy link
Member Author

ronag commented Nov 14, 2022

Tricky to benchmark due to async stuff. We would need to measure cpu utilization instead of ops per second.

@ronag ronag changed the title Atomic condition variable is very slow in V8 Atomic condition variable Nov 14, 2022
@anonrig anonrig added the benchmark-needed Add to issues that does not have a benchmark label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark-needed Add to issues that does not have a benchmark
Projects
None yet
Development

No branches or pull requests

2 participants