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.

@coolreader18 coolreader18 added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@bfops bfops added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@bfops bfops added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@bfops bfops added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@bfops bfops added this pull request to the merge queue Feb 2, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2026
@bfops bfops added this pull request to the merge queue Feb 2, 2026
Merged via the queue into master with commit 5dc86a0 Feb 3, 2026
45 of 47 checks passed
@coolreader18 coolreader18 deleted the noa/v8-corepinning branch February 3, 2026 03:18
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