Move execution ack off of the critical path#4816
Draft
joshua-spacetime wants to merge 1 commit intojoshua/keep-subscription-fanout-busyfrom
Draft
Move execution ack off of the critical path#4816joshua-spacetime wants to merge 1 commit intojoshua/keep-subscription-fanout-busyfrom
joshua-spacetime wants to merge 1 commit intojoshua/keep-subscription-fanout-busyfrom
Conversation
Centril
approved these changes
Apr 20, 2026
Contributor
Centril
left a comment
Centril
left a comment
There was a problem hiding this comment.
Looks OK, just some nits.
I'm fine with merging this in the interest of perf. It's not that bad for now.
| } => { | ||
| let res = instance_common.update_database(program, old_module_info, policy, &mut inst); | ||
| send_worker_reply("update_database", reply_tx, res); | ||
| send_worker_reply(reply_relay.as_ref(), "update_database", reply_tx, res); |
Contributor
There was a problem hiding this comment.
Can you do this once at the top of the function so that the .as_ref() noise is removed?
| // Convert program to a string. | ||
| let program: Arc<str> = str::from_utf8(program_bytes)?.into(); | ||
| let database_identity = mcc.replica_ctx.database_identity; | ||
| let lane_queue = JsWorkerQueue::unbounded_with_metric( |
Contributor
There was a problem hiding this comment.
Suggested change
| let lane_queue = JsWorkerQueue::unbounded_with_metric( |
The paragraph comment "Convert program to a string." now also includes unrelated code. Let's fix that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Today, for each client connection, we process requests one at a time, and we wait for each request to finish before processing the next one. The way this works is that each websocket reader waits for a notification (via a one shot channel) from the JS worker that it is done processing the request. This notification is quite costly because it always wakes the receiving task.
To avoid this costly wake, this patch adds an intermediate relay actor. The relay reads from its message queue periodically and forwards the replies on to the correct websocket reader task. This trades latency for increased throughput.
I would classify this as a hack, and so I'm not sure I would recommend merging this. A potentially better solution would be to remove the reply altogether. However if we want to preserve execution order per connection, I'm not sure if this is possible today, because I'm not sure if we have a single serialization point for all requests. But I think it is ultimately the direction we want to go.
API and ABI breaking changes
None
Expected complexity level and risk
1
Testing
Manual