[ZEPPELIN-6700] Convert a broadcast message to JSON once instead of once per connection - #5462
Open
big-cir wants to merge 1 commit into
Open
[ZEPPELIN-6700] Convert a broadcast message to JSON once instead of once per connection#5462big-cir wants to merge 1 commit into
big-cir wants to merge 1 commit into
Conversation
…nce per connection
tbonelee
approved these changes
Sep 6, 2026
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
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.
What is this PR for?
ConnectionManagerconverts aMessageto JSON usingserializeMessageand writes the result to a socket. A broadcast sends one message to many connections, so the two steps have different multiplicity: the JSON is one value, the writes are many. Five call sites currently perform the conversion inside the per-connection loop, causing it to run once per connection instead of once per message:broadcast(Message), inside thesynchronized (connectedSockets)blockbroadcast(String, Message)broadcastToWatchers(String, String, Message), itself reached once per broadcast frombroadcast,broadcastExcept, andunicastbroadcastExcept(String, Message, NotebookSocket), the path used by collaborative patches, Angular object updates, and spell resultsunicast(Message, NotebookSocket), reached once per connection frommulticastToUser, which serves both note-list updates and personalized-mode paragraphsThe message is not mutated inside these loops, and
gson.toJsonis deterministic. Every iteration therefore produces a byte-identical string, and all but one are discarded.There is no functional bug being fixed here. The bytes on the wire and their order are already correct, and on a note with a single connection the current form costs nothing extra. What it does is repeat work in proportion to the number of connections attached to a note, and three paths make that repetition routine rather than occasional:
Paragraphincluding its output, whichzeppelin.interpreter.output.limitcaps at 100 KB by default. This is the path where a single redundant conversion is expensive.AppendOutputRunnerflush. Each message carries only the appended chunk and the runner coalesces writes over a 100 ms window, so these are small but numerous.The features that make a note worth sharing are the ones that pay for this most.
This PR converts once per broadcast at those five sites and writes that one string to every connection. The resulting JSON and write order remain unchanged. Two supporting changes come with it:
broadcastToWatchersreturns early when no watcher is attached, so hoisting the conversion out of its loop does not introduce work in the common case where the loop body never ran.multicastToUsernow sends directly rather than delegating tounicast, becauseunicastbundles the conversion with a watcher broadcast. The watcher broadcast stays inside the loop, so watchers receive the same messages they do today.One behavioral detail is worth flagging for review. Hoisting moves the conversion out of the per-connection
trythat exists to catchIOExceptionfromNotebookSocket.send. A conversion failure was logged and skipped per connection before, and now propagates to the caller. That handler was incidental tosendrather than an intentional contract for the conversion, and it never caughtStackOverflowError, which is the likely failure mode for a cyclic object graph.NotebookServeris untouched. Its ownserializeMessageis used for single sends, and the one loop there that converts per iteration builds a different message each time, so there is nothing to hoist.What type of PR is it?
Improvement
Todos
broadcast(Message),broadcast(String, Message),broadcastExcept, andbroadcastToWatchersbroadcastToWatcherswhen no watcher is attachedmulticastToUserinstead of going throughunicast, keeping the watcher broadcast inside the loopWhat is the Jira issue?
How should this be tested?
Three unit tests were added to
ConnectionManagerTest. They subclassConnectionManagerto count how often a broadcast converts its message and assert that every connection receives an identical payload.Screenshots (if appropriate)
N/A
Questions: