⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@coolreader18
Copy link
Collaborator

Description of Changes

This got a little bigger than I had hoped, but I think it's still pretty manageable. This PR partially reverts back to before #3263: cores are re-balanced with a watch::Receiver<CoreId> that each database thread will listen for updates on in order to repin itself, and multiple OS threads (each matched to a database) can be pinned to one core. As I understand it, that second part is something Phoebe was trying to avoid, but given that there's no way to asyncify a JS module, it's kind of necessary.

JS is single-threaded, and uses cooperative rather than preemptive multitasking (callbacks/async, not green threads). That means that if a JS function has an infinite loop, no other event handlers would be able to run unless that loop were to exit. Coupled with the fact that we can't Send a v8 isolate across threads, it makes more sense to keep the module host on one thread and repin that thread as needed. An alternative option, as was brought up, would be to deconstruct and reconstruct the module onto a different thread when needed, since load-balancing won't be happening often anyway.

Expected complexity level and risk

3 - reworks the threadpool that databases run on, and so could lead to deadlocks or other concurrency bugs. However, that seems unlikely, since this separates databases each onto their own thread, and as such decreases the likelihood of them interacting poorly with each other.

Testing

Not sure if there's anything specific I should do, since this doesn't change behavior.

@coolreader18 coolreader18 requested a review from gefjon January 26, 2026 21:02
@coolreader18 coolreader18 force-pushed the noa/v8-corepinning branch 2 times, most recently from 3508fe9 to a284e6c Compare January 26, 2026 21:57
@coolreader18
Copy link
Collaborator Author

Can confirm that this improves performance; just this patchset brings v8 from 78.5k TPS to 89k TPS.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some requests for more docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants