Rework `JobCores` in order to core-pin v8 instance threads #4128

coolreader18 · 2026-01-26T21:02:37Z

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 · 2026-01-26T22:02:22Z

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

Centril

Just some requests for more docs.

Centril · 2026-01-27T15:55:49Z

crates/core/src/host/v8/mod.rs

+        }
+    }
+
+    pub async fn run_on_thread<F, R>(&mut self, f: F) -> R


Some documentation would be good here.

Centril · 2026-01-27T15:56:22Z

crates/core/src/host/v8/mod.rs

 // We care about optimizing for `CallReducer` as it happens frequently,
 // so we don't want to box anything in it.
 enum JsWorkerRequest {
+    /// See [`JsInstance::run_on_thread`]


Suggested change

/// See [`JsInstance::run_on_thread`]

/// See [`JsInstance::run_on_thread`].

Centril · 2026-01-27T15:57:24Z

crates/core/src/host/v8/mod.rs

+    load_balance_guard: Arc<LoadBalanceOnDropGuard>,
+    mut core_pinner: CorePinner,


These parameters could be incorporated into the function docs.

Centril · 2026-01-27T16:09:41Z

crates/core/src/util/jobs.rs

+#[derive(Default)]
+pub struct AllocatedJobCore {
+    pub guard: LoadBalanceOnDropGuard,
+    pub pinner: CorePinner,
+}
+
+impl AllocatedJobCore {
+    pub fn spawn_async_executor(self) -> SingleCoreExecutor {
+        SingleCoreExecutor::spawn(self)
+    }
+}
+
+#[derive(Default, Clone)]
+pub struct CorePinner {
+    move_core_rx: Option<watch::Receiver<CoreId>>,
+}
+
+impl CorePinner {
+    pub fn pin_now(&mut self) {
+        if let Some(move_core_rx) = &mut self.move_core_rx {
+            let core_id = *move_core_rx.borrow_and_update();
+            core_affinity::set_for_current(core_id);
+        }
+    }
+    pub fn pin_if_changed(&mut self) {
+        if let Some(move_core_rx) = &mut self.move_core_rx {
+            if let Ok(true) = move_core_rx.has_changed() {
+                let core_id = *move_core_rx.borrow_and_update();
+                core_affinity::set_for_current(core_id);
+            }
+        }
+    }
+    pub async fn run(self) {
+        if let Some(mut move_core_rx) = self.move_core_rx {
+            while move_core_rx.changed().await.is_ok() {
+                let core_id = *move_core_rx.borrow_and_update();
+                core_affinity::set_for_current(core_id);
+            }
+        }
+    }
+}


Docs on these please :)

gefjon · 2026-01-27T20:05:11Z

crates/core/src/util/jobs.rs

 impl PinnedCoresExecutorManager {
-    /// Get a [`runtime::Handle`] for running database operations on,
+    /// Get a core for running database operations on,
    /// and store state in `self` necessary to move that database to a new runtime


This doc comment needs to be changed; databases no longer move between runtimes.

gefjon · 2026-01-27T20:08:01Z

crates/core/src/host/module_host.rs

+}
+
+/// A module; used as a bound on `InstanceManager`.
+trait GenericModule {


Why do we need a trait here? There's only two kinds of module, not an unbounded set of them.

gefjon · 2026-01-27T20:09:12Z

crates/core/src/host/module_host.rs

+struct ModuleInstanceManager<M: GenericModule> {
+    instances: Mutex<VecDeque<M::Instance>>,
+    module: M,


Why are we rewriting this to use trait polymorphism rather than tagged enums?

It more accurately models the data it holds. We're never going to have a ModuleInstanceManager where the module is Wasm but the instances are Js, or where the elements of instances are heterogenous. Now we don't have to do let Instance::Js(inst) = inst else { unreachable!() };, and as a nice side benefit the VecDeque is more compact (again because of the type system–enforced homogeneity).

I don't object to this specifically. My main concern here is that we not regress to the previous version of instance managers which used dyn Trait runtime polymorphism.

In general, I urge you to keep an eye on when using more complex types to more accurately model data provides a tangible benefit, versus when it adds needless complexity. In this case, I do not think that let inst = inst.into_js().unwrap(); would be likely to lead to confusion or bugs, nor do I believe that reducing the VecDeque's size is likely to tangibly affect our performance.

In this particular case, I agree with @coolreader18. Enforcing the common interface is useful and keeps things in sync.
Type classes are not just about polymorphism, but also about the restrictions they impose and the "laws" that come with them.

Centril · 2026-01-28T14:07:09Z

crates/core/src/util/jobs.rs

+                let core_id = *move_core_rx.borrow_and_update();
+                core_affinity::set_for_current(core_id);


Also, could you make a function that does this to not duplicate 3x? Make it #[inline] also please.

Centril · 2026-01-28T14:07:57Z

crates/core/src/util/jobs.rs

+            core_affinity::set_for_current(core_id);
+        }
+    }
+    pub fn pin_if_changed(&mut self) {


Using this turns out to be critical for performance. The difference is about 6k TPS.

coolreader18 requested a review from gefjon January 26, 2026 21:02

coolreader18 force-pushed the noa/v8-corepinning branch from e6209fb to 3508fe9 Compare January 26, 2026 21:04

coolreader18 added 2 commits January 26, 2026 15:57

Rework JobCores in order to pin v8 instance threads

89eeba9

Run http_request on a tokio task

a284e6c

coolreader18 force-pushed the noa/v8-corepinning branch from 3508fe9 to a284e6c Compare January 26, 2026 21:57

Centril reviewed Jan 27, 2026

View reviewed changes

gefjon reviewed Jan 27, 2026

View reviewed changes

Centril reviewed Jan 28, 2026

View reviewed changes

	/// See [`JsInstance::run_on_thread`]
	/// See [`JsInstance::run_on_thread`].

		load_balance_guard: Arc<LoadBalanceOnDropGuard>,
		mut core_pinner: CorePinner,

		let core_id = *move_core_rx.borrow_and_update();
		core_affinity::set_for_current(core_id);

Rework JobCores in order to core-pin v8 instance threads #4128

Are you sure you want to change the base?

Rework JobCores in order to core-pin v8 instance threads #4128

Uh oh!

Conversation

coolreader18 commented Jan 26, 2026

Description of Changes

Expected complexity level and risk

Testing

Uh oh!

coolreader18 commented Jan 26, 2026

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rework `JobCores` in order to core-pin v8 instance threads #4128

Rework `JobCores` in order to core-pin v8 instance threads #4128