batch/debounce websocket set_props#3783
Conversation
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
Could you add a changelog entry? I'd also like to see a test related to the new batching mechanism.
| * Groups set_props by renderer and forwards as a single batch message. | ||
| * @param messages Array of messages | ||
| */ | ||
| private handleBatchedMessages(messages: unknown[]): void { |
There was a problem hiding this comment.
Why is messages of type unknown[]? Why not WorkerMessages[]?
| if (!setPropsPayloadsByRenderer.has(rendererId)) { | ||
| setPropsPayloadsByRenderer.set(rendererId, []); | ||
| } | ||
| setPropsPayloadsByRenderer.get(rendererId)!.push(setPropsMsg.payload); |
There was a problem hiding this comment.
Non null assertions are a code smell. Could this be rewritten to avoid it?
| // Forward batched set_props to each renderer | ||
| for (const [rendererId, payloads] of setPropsPayloadsByRenderer) { | ||
| const port = this.renderers.get(rendererId); | ||
| if (port) { |
There was a problem hiding this comment.
Should a message be logged if port is falsy?
There was a problem hiding this comment.
Does the message order matter? If you batch everything, the order will change. [set_prop_1, otherMessage, set_prop_2] gets sent out in the order of [set_prop_1, set_prop_2], [otherMessage].
| Args: | ||
| send_text: Async function to send text data over WebSocket | ||
| outbound_queue: janus.Queue instance for receiving messages (strings) | ||
| batch_delay: Time in seconds to wait for additional messages (default: 5ms) |
There was a problem hiding this comment.
The OP says use 0 if you want to disable batching. Maybe that should be included here? Would None be a better option? Would it be better to call say this disables debouncing rather than disabling batching?
There was a problem hiding this comment.
0 matches the same pattern as the inactivity config. Debouncing is the just the mechanism by which the batching happens, so batching is more relevant than debounce imo.
2 participants
When you use set_props in a loop inside a websocket callback, it would create single updates in the renderer and can result in lag.
This PR add a batching and debounce mechanism to set_props, default to 5ms/200hz configurable with