perf: eliminate per-message topic string alloc in comms receive path - #9794
perf: eliminate per-message topic string alloc in comms receive path#9794alejandro-jimenez-dcl wants to merge 5 commits into
Conversation
## Problem Every incoming LiveKit `CommsData` message allocates a temporary managed topic string on the LiveKit callback thread, purely to key a dictionary lookup — including messages for topics the scene never subscribed to (all scene-bound CommsData traffic reaches every `CommsApiWrap` handler). In busy scenes this is a steady off-main-thread GC-pressure stream. This is the exact TODO tracked by #9206 ("implement GetAlternateLookup ... to avoid allocation of temp string instances"). ## Root cause Topic identity arrives as UTF-8 bytes inside the wire span, but the subscription registry is only addressable by `string`, and `GetAlternateLookup` (span-keyed dictionary lookup) is a .NET 9 API that Unity 6000.4's BCL does not have — so the hot receive path materialized the key per message. The registry mutates only via rare JS-driven subscribe/unsubscribe and scenes use a handful of topics. ## Fix (~55 LOC, single file) `CommsApiWrap`: a byte-keyed immutable snapshot (`TopicLookupEntry[]` — UTF-8 topic bytes + shared queue ref) rebuilt under a lock on successful subscribe/unsubscribe and published via a volatile field. `OnDataReceived` volatile-reads the snapshot and linear-scans with `SequenceEqual` on the topic span — zero allocation on the miss path; the data-string alloc on the subscribed branch is required (handed to JS) and unchanged. Copy-on-write publication means readers never see a partial snapshot; the string-keyed JS-side `ConsumeMessages` shares the same queues and is untouched. ## Test In the existing `CommsApiWrapShould` harness: - `ReceiveForUnsubscribedTopicDoesNotAllocate` — budgeted GC.Alloc profiler-recorder window (same idiom as EventBusShould) with a liveness canary: pin allocates ~1 topic string per message; fix allocates none. - `ResubscribeAfterUnsubscribeReceivesAgain` — snapshot-rebuild correctness on both mutation paths. ## Validation Windows Unity 6000.4 EditMode lane at the pin: RED FAIL as intended (1016 GC.Alloc samples over 1000 messages vs a <100 budget — exactly one topic-string alloc per message plus the 16-alloc canary; the functional companion passed at pin) / GREEN PASS 2/2. The alloc probe was hardened during validation (thread-local byte counters are inert on the editor Mono runtime; the shipped test uses the GC.Alloc recorder + canary idiom). Fixes #9206 Includes inspection-warning cleanup in all touched files.
🚦 CI Status
Build skipped — no changes detected under No C# files changed — lint ratchet skipped. All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Step 2 — Root-cause check: PASS
The PR fixes the root cause of #9206: per-message string allocation in OnDataReceived caused by Encoding.UTF8.GetString being the only way to key into the string-keyed topicBuffers dictionary, with GetAlternateLookup (span-keyed lookup) being a .NET 9 API unavailable in Unity 6000.4's BCL. The fix introduces a byte-keyed copy-on-write snapshot for zero-allocation topic matching — a direct fix, not a workaround or symptom suppression.
Step 3 — Design & integration: PASS
Is this a new long-lived unit managing a lifecycle? No. TopicLookupEntry[] is an internal derived index within CommsApiWrap, not a standalone manager/system/controller. It does not own or manage any entity, scene, or resource lifecycle — it is a read-optimization of the existing topic subscription registry.
Owner search: CommsApiWrap itself is the owner of topic subscriptions. It is constructed by the scene runtime's JS module system and disposed when the scene unloads. The new topicLookup snapshot shares the same lifecycle — initialized in the field declaration, rebuilt on subscribe/unsubscribe, cleared in Dispose(). No new lifecycle is introduced.
Is topicBuffers still needed alongside topicLookup? Yes — topicBuffers remains the string-keyed source of truth for ConsumeMessages (JS-facing, takes string topic) and for the TryAdd/TryRemove existence checks in subscribe/unsubscribe. topicLookup serves a genuinely different consumer (wire-format byte matching on the LiveKit thread). This is not a redundant-state anti-pattern.
Copy-on-write array pattern: Appropriate for the read/write ratio — extremely hot reads on the LiveKit callback thread, rare writes from JS subscribe/unsubscribe. Writers serialize under topicLookupLock; readers take a zero-cost volatile snapshot. The shared DCLConcurrentQueue references ensure messages enqueued via the byte-keyed path are visible to string-keyed ConsumeMessages.
Teardown trace:
topicLookup→ set toArray.Empty<>()inDispose()(line 73) ✓topicLookupLock→ plainobject, no disposal needed ✓TopicLookupEntry.Utf8Topic(byte[]) → GC-managed, no explicit disposal needed ✓TopicLookupEntry.Queue→ shared reference to queue intopicBuffers, cleared viatopicBuffers.Clear()✓
Step 4 — Member audit
New public members: None. TopicLookupEntry and RebuildTopicLookup() are both private.
Changed public member: PublishData(string topic, string? data) — the data parameter changed from string to string?. This is a nullable annotation correction, not a behavioral change: the method body already handled null via if (string.IsNullOrEmpty(data)) return; (unchanged), and the existing RejectNullData test (also unchanged) asserted this behavior. The annotation now truthfully reflects what the ClearScript/JS boundary can pass.
Step 5 — Line-level findings
One P2 finding (inline comment below).
Security: No vulnerabilities. Input validation in OnDataReceived (span length checks before slicing) is unchanged and correct. The volatile + lock COW pattern is correctly implemented. TopicLookupEntry.Utf8Topic (byte[]) is created inside RebuildTopicLookup and never exposed outside the private nested type.
Thread safety: Correct. Writers serialize under topicLookupLock; the LiveKit-thread reader takes a volatile snapshot. The self-healing property of full-rebuild-from-dictionary means concurrent subscribe/unsubscribe calls always converge to the correct state.
Performance: The allocation elimination is effective and complete for the unsubscribed-topic path. SequenceEqual on ReadOnlySpan<byte> vs byte[] (implicit conversion to ReadOnlySpan<byte>) is allocation-free. The linear scan is appropriate for the expected cardinality (a handful of topics per scene).
Tests: Both new tests follow the AAA pattern, use NUnit conventions, and cover the key correctness properties: zero-alloc verification with a liveness canary (sound GC.Alloc Recorder methodology), and snapshot-rebuild correctness across unsubscribe + resubscribe.
Step 6 — Complexity: COMPLEX
Touches the LiveKit callback thread's message-processing path and involves concurrent data structure design (volatile + lock COW pattern). Memory management optimization targeting GC pressure.
Step 7 — QA: YES
Modifies runtime comms message processing code that ships in the Unity player build.
Step 8 — Non-blocking warnings
main but the repository's default branch is dev. Verify this is intentional per the current branch model.
Step 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches the LiveKit-thread comms receive path with a copy-on-write concurrent data structure for GC-pressure elimination
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: perf: eliminate per-message topic string alloc in comms receive path
STEP 2 — Root-cause check: ✅ PASS
The PR correctly identifies and fixes the root cause: topic identity arrives as UTF-8 bytes on the wire, but the subscription registry was only addressable by string, and GetAlternateLookup (span-keyed dictionary lookup) is a .NET 9 API unavailable in Unity 6000.4's BCL — forcing a per-message string materialization purely for a dictionary key lookup. The fix uses a copy-on-write byte-keyed snapshot for zero-allocation matching on the hot path. This is the right approach given the platform constraint.
STEP 3 — Design & integration: ✅ PASS
New unit assessment: No new system, manager, or service is introduced. The change adds a private field (topicLookup), a private method (RebuildTopicLookup), and a private readonly struct (TopicLookupEntry) — all scoped within the existing CommsApiWrap class. The lifecycle is managed by the same class that owns topicBuffers.
Owner search: CommsApiWrap is constructed in ISceneRuntime.cs (line 175, via SceneFactory), registered as the "CommsApi" JS module, and disposed on scene unload. The snapshot lifecycle is tied to subscribe/unsubscribe calls (JS-driven, rare) and the class's Dispose(). No external lifecycle owner is needed — the existing class is the correct home for this optimization.
Dual data structure consistency: topicBuffers (string-keyed, used by ConsumeMessages on JS thread) and topicLookup (byte-keyed, used by OnDataReceived on LiveKit thread) share the same DCLConcurrentQueue references. Writers rebuild topicLookup under topicLookupLock immediately after mutating topicBuffers. The brief desync window (sub-microsecond) is inherent to copy-on-write and self-correcting. At worst, a message on a just-subscribed topic is missed during the rebuild window, or a message on a just-unsubscribed topic is enqueued to an unreachable queue. Both are benign and transient.
Teardown trace:
| Acquired | Teardown | Location |
|---|---|---|
onDataReceivedCached handler registration |
RemoveSceneMessageHandler |
Dispose() line 71 |
topicBuffers entries |
topicBuffers.Clear() |
Dispose() line 72 |
topicLookup snapshot |
topicLookup = Array.Empty<>() |
Dispose() line 73 |
commsWriter |
commsWriter.Dispose() |
Dispose() line 75 |
All acquired resources have matching teardown. No leaks.
Dispose ordering note (not a finding): Dispose() resets topicLookup without acquiring topicLookupLock. A theoretical race exists if a concurrent SubscribeToTopic triggers RebuildTopicLookup between topicBuffers.Clear() (line 72) and the volatile write (line 73). This is harmless in practice: the handler is removed first (line 71) so no new OnDataReceived calls arrive, and any in-flight call is working with an already-captured snapshot. Post-Dispose subscribe calls would be a lifecycle violation by the caller.
STEP 4 — Member audit: ✅ PASS
| New member | Consumers | Assessment |
|---|---|---|
topicLookupLock (private) |
RebuildTopicLookup |
Standard lock object for serializing snapshot rebuilds |
topicLookup (private volatile) |
OnDataReceived (read), RebuildTopicLookup (write), Dispose (write) |
Correct volatile publish/read pattern |
RebuildTopicLookup() (private) |
SubscribeToTopic, UnsubscribeFromTopic |
Two callers — appropriate extraction |
TopicLookupEntry (private readonly struct) |
topicLookup, RebuildTopicLookup, OnDataReceived |
Clean value type, correct readonly struct usage |
No single-use merge candidates. No absent≠false issues. No redundant guards.
STEP 5 — Line-level review: ✅ No issues found
Threading model: The copy-on-write pattern (volatile publish for readers, lock for writers) is textbook for this access profile — rare writers (JS subscribe/unsubscribe), frequent readers (LiveKit callback thread). ConcurrentDictionary enumeration in RebuildTopicLookup is snapshot-safe on non-WebGL. On WebGL (single-threaded), no concurrency concern.
Hot path allocation audit: OnDataReceived is allocation-free on the miss path (unsubscribed topics). On the hit path, only the intentional Encoding.UTF8.GetString (line 299, data string handed to JS) and the BufferedDataMessage struct enqueue (no boxing — generic concurrent queue) allocate. No LINQ, no closures, no hidden allocations.
Input validation: Wire format bounds are correctly validated (lines 274, 278) before any span slicing. topicLength as ushort combined with the length check prevents out-of-bounds access.
Signature change: PublishData(string topic, string? data) correctly aligns the nullable annotation with actual behavior. The string.IsNullOrEmpty(data) guard (line 146) handles both null and empty, with test coverage (RejectNullData, RejectEmptyData).
Linear scan correctness: The for loop with SequenceEqual + return on match (lines 286–302) is functionally equivalent to the previous TryGetValue — topics are unique (sourced from a dictionary) so at most one match exists. The return after the match correctly short-circuits the scan.
Security: No new attack surface introduced. Wire format validation is unchanged and sound. The byte[] in TopicLookupEntry is never exposed outside the private readonly struct and only read via SequenceEqual — no mutation path exists.
Tests: Both new tests follow AAA pattern, use NUnit correctly, and cover the optimization's two key properties:
ReceiveForUnsubscribedTopicDoesNotAllocate: GC.Alloc Recorder with liveness canary — validates zero allocation on the miss pathResubscribeAfterUnsubscribeReceivesAgain: Snapshot rebuild correctness on both mutation paths (subscribe and unsubscribe)
null! usage on test fields (lines 21–25) is the standard pattern for [SetUp]-initialized fields — acceptable per convention.
STEP 6 — Complexity: COMPLEX
Touches the LiveKit comms receive path threading model and memory allocation patterns on the callback thread.
STEP 7 — QA: YES
Modifies runtime code under Explorer/ that ships in the build. The change affects how incoming CommsData messages are matched to subscribed topics on the LiveKit callback thread.
STEP 8 — Warnings: None
Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the LiveKit comms receive path threading model and allocation patterns on the callback thread
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
PR #9794, run #32262975226 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
alejandro-jimenez-dcl
left a comment
alejandro-jimenez-dcl
left a comment
There was a problem hiding this comment.
approved
3 participants
Problem
Every incoming LiveKit
CommsDatamessage allocates a temporary managed topic string on theLiveKit callback thread, purely to key a dictionary lookup - including messages for topics
the scene never subscribed to (all scene-bound CommsData traffic reaches every
CommsApiWraphandler). In busy scenes this is a steady off-main-thread GC-pressurestream. This is the exact TODO tracked by #9206 ("implement GetAlternateLookup ... to avoid
allocation of temp string instances").
Root cause
Topic identity arrives as UTF-8 bytes inside the wire span, but the subscription registry
is only addressable by
string, andGetAlternateLookup(span-keyed dictionary lookup) isa .NET 9 API that Unity 6000.4's BCL does not have - so the hot receive path materialized
the key per message. The registry mutates only via rare JS-driven subscribe/unsubscribe and
scenes use a handful of topics.
Fix (~55 LOC, single file)
CommsApiWrap: a byte-keyed immutable snapshot (TopicLookupEntry[]- UTF-8 topic bytes +shared queue ref) rebuilt under a lock on successful subscribe/unsubscribe and published
via a volatile field.
OnDataReceivedvolatile-reads the snapshot and linear-scans withSequenceEqualon the topic span - zero allocation on the miss path; the data-string allocon the subscribed branch is required (handed to JS) and unchanged. Copy-on-write
publication means readers never see a partial snapshot; the string-keyed JS-side
ConsumeMessagesshares the same queues and is untouched.Test
In the existing
CommsApiWrapShouldharness:ReceiveForUnsubscribedTopicDoesNotAllocate- budgeted GC.Alloc profiler-recorder window(same idiom as EventBusShould) with a liveness canary: pin allocates ~1 topic string per
message; fix allocates none.
ResubscribeAfterUnsubscribeReceivesAgain- snapshot-rebuild correctness on bothmutation paths.
Validation
Windows Unity 6000.4 EditMode lane at the pin: RED FAIL as intended (1016 GC.Alloc samples
over 1000 messages vs a <100 budget - exactly one topic-string alloc per message plus the
16-alloc canary; the functional companion passed at pin) / GREEN PASS 2/2. The alloc probe
was hardened during validation (thread-local byte counters are inert on the editor Mono
runtime; the shipped test uses the GC.Alloc recorder + canary idiom).
Fixes #9206
Includes inspection-warning cleanup in all touched files.
Fixes #9206