perf: remove per-poll List allocation in notifications polling - #9804
perf: remove per-poll List allocation in notifications polling#9804alejandro-jimenez-dcl wants to merge 1 commit into
Conversation
## Problem The notifications poller allocates a fresh `List<INotification>` plus a full Newtonsoft LINQ-to-JSON object graph every 5 seconds for the entire app lifetime, even when zero notifications arrive — steady-state GC churn on the hottest always-on polling loop in the client. Tracked by the in-code TODO and tech-debt issue #9263. ## Root cause Newtonsoft's `JsonSerializer.Populate` never consults root-level `JsonConverter`s, and the notifications payload is an object wrapping an interface-typed list only `NotificationJsonDtoConverter` can parse — so the existing reuse path (`OverwriteFromNewtonsoftJsonAsync`) was unusable for this endpoint and the poll loop was stuck on the allocating create-op. The converter's own `existingValue` support was dead code. ## Fix (~30 LOC) - `GenericDownloadHandlerUtils`: new `PopulateInto<T>(JsonReader, T target, JsonSerializer)` — routes the first matching registered converter with the target as `existingValue`, falling back to `serializer.Populate` (existing behavior). The Overwrite op's Newtonsoft branch now uses it, making converter-aware reuse available to every poller. - `NotificationsRequestController`: a reusable `pollNotificationsBuffer` cleared per iteration + `OverwriteFromNewtonsoftJsonAsync`; the per-poll URL interpolation is hoisted to a ctor-built field. `GetMostRecentNotificationsAsync` is deliberately not pooled (its list escapes to the panel controller). Contract blast radius: behavior changes only when a registered converter matches the ROOT target type of an Overwrite call — a combination that today either throws or silently ignores the converter; all existing callers audited unaffected. ## Test - `NotificationsRequestControllerShould.ReuseSingleListInstanceAcrossPollIterations` — drives two poll iterations against a stubbed web controller; asserts the overwrite op is used, the same `Target` instance is passed both iterations, and the buffer is empty at delivery (the re-dispatch hazard). - `GenericDownloadHandlerPopulateIntoShould` (3 tests) — converter-routed populate in place, no duplication after `Clear()`, and the no-matching-converter fallback. ## Validation Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 1/1 as intended (overwrite op received 0 calls at pin — the loop allocates a fresh list per poll) / GREEN PASS 4/4. Side-find from validation (separate issue): a real wire "events_ended" notification would throw `JsonSerializationException` in production `ReadJson` — `NotificationType` has no matching enum member. Fixes #9263 Includes inspection-warning cleanup in all touched files.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #9804 perf: remove per-poll List allocation in notifications polling
STEP 2 — Root-cause check: PASS
The problem is that JsonSerializer.Populate never consults root-level JsonConverters, so NotificationJsonDtoConverter was unreachable from the existing OverwriteFromNewtonsoftJsonAsync path. The fix introduces PopulateInto<T> to route matching root-level converters manually, making the reuse-op generically correct — not just a workaround for notifications. This addresses the cause (converter-unaware Populate), not a symptom.
STEP 3 — Design & integration: PASS
No new long-lived units introduced. The pollNotificationsBuffer is a method-local variable scoped to StartGettingNewNotificationsOverTimeAsync — it doesn't escape the loop (items are dispatched individually to NotificationsBusController). The notificationsUrl field is a simple readonly cache of a previously per-call allocation.
PopulateInto<T> lives in GenericDownloadHandlerUtils, which already owns all JSON deserialization plumbing for the web-request framework. This is the correct home — it fixes a general gap in OverwriteFromJsonAsyncOp, not a notifications-specific one.
Teardown trace: No subscriptions, event hookups, or disposables are added by this diff. The buffer is a plain List<> cleared each iteration; no teardown needed.
Existing caller impact verified: Searched all OverwriteFromNewtonsoftJsonAsync / OverwriteFromJsonAsync callers:
ProfilesRequest.cs— usesSERIALIZER_SETTINGSwithProfileConverter/ProfileCompactInfoConverter, but the root target type isIList<Profile>, which neither converter matches → falls through toserializer.Populate()(identical to prior behavior).PlacesAPIClient.cs/RealmController.cs— useWRJsonParser.Unitypath (JsonUtility.FromJsonOverwrite),PopulateIntois never called.LoadElementsByPointersSystem.cs— usesWRJsonParser.Newtonsoftwith default settings (no custom converters) → falls through toserializer.Populate().
All existing callers are unaffected.
STEP 4 — Member audit: PASS
PopulateInto<T> (public static) — 1 direct consumer (OverwriteFromJsonAsyncOp.ExecuteAsync). Public because it completes the Overwrite op's contract for any future caller with a root-level converter. Not single-use indirection — it fills a real gap that would otherwise require each caller to duplicate the converter-routing logic.
notificationsUrl (private readonly) — 2 consumers within the class (GetMostRecentNotificationsAsync, StartGettingNewNotificationsOverTimeAsync). Replaces per-call URLDomain.FromString(string-interpolation) with a ctor-cached value.
STEP 5 — Line-level review
See inline comments below.
STEP 6 — Complexity: COMPLEX
Touches the shared JSON deserialization infrastructure (GenericDownloadHandlerUtils) used by all web-request pollers, and modifies an async polling loop with converter-aware reuse semantics.
STEP 7 — QA: YES
Modifies runtime code under Explorer/ that ships in the build (notifications polling path).
STEP 8 — Non-blocking warnings
None. Main scene is not modified.
Security review
No security issues found. No new external input handling, no secrets, no auth changes. Error handling patterns preserved.
Test coverage
Excellent. 6 tests covering:
- Buffer reuse across poll iterations (controller integration test)
- Converter-routed populate (happy path)
- Clear + reuse (no duplication)
- Fresh-instance safety check (throws)
- Null body via converter null guard
- Fallback to
serializer.Populatewhen no converter matches
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies shared JSON deserialization infrastructure (GenericDownloadHandlerUtils) and converter-aware reuse path for the web-request framework
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| if (reader.TokenType == JsonToken.None) | ||
| reader.Read(); |
There was a problem hiding this comment.
[P2] Defensive: reader.Read() return value is discarded. If a truncated or empty stream reaches this point, Read() returns false and the converter receives a reader still at JsonToken.None, producing a confusing downstream error from JObject.Load instead of a clear failure. In practice this is unlikely (empty HTTP bodies are caught earlier), but a defensive check here would surface the issue at its origin.
| if (reader.TokenType == JsonToken.None) | |
| reader.Read(); | |
| if (reader.TokenType == JsonToken.None && !reader.Read()) | |
| throw new JsonSerializationException("Unexpected end of JSON input; cannot populate target."); |
| { | ||
| JsonConverter converter = converters[i]; | ||
|
|
||
| if (converter.CanRead && converter.CanConvert(typeof(T))) |
There was a problem hiding this comment.
[P2] Style: evaluation order. Newtonsoft internally evaluates CanConvert(type) first (the selective type gate), then CanRead (a broad flag that defaults to true). Swapping to match the conventional order improves readability for anyone familiar with Newtonsoft internals, and short-circuits on the cheaper type check when converters opt out of specific types.
| if (converter.CanRead && converter.CanConvert(typeof(T))) | |
| if (converter.CanConvert(typeof(T)) && converter.CanRead) |
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12724 => 13131 — remove at least 408 warnings to merge. Warnings/errors in files changed by this PR (2)All Unity tests passed ✅
|
|
PR #9804, run #32268989339 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
3 participants
Problem
The notifications poller allocates a fresh
List<INotification>plus a full NewtonsoftLINQ-to-JSON object graph every 5 seconds for the entire app lifetime, even when zero
notifications arrive - steady-state GC churn on the hottest always-on polling loop in the
client. Tracked by the in-code TODO and tech-debt issue #9263.
Root cause
Newtonsoft's
JsonSerializer.Populatenever consults root-levelJsonConverters, and thenotifications payload is an object wrapping an interface-typed list only
NotificationJsonDtoConvertercan parse - so the existing reuse path(
OverwriteFromNewtonsoftJsonAsync) was unusable for this endpoint and the poll loop wasstuck on the allocating create-op. The converter's own
existingValuesupport was deadcode.
Fix (~30 LOC)
GenericDownloadHandlerUtils: newPopulateInto<T>(JsonReader, T target, JsonSerializer)existingValue,falling back to
serializer.Populate(existing behavior). The Overwrite op's Newtonsoftbranch now uses it, making converter-aware reuse available to every poller.
NotificationsRequestController: a reusablepollNotificationsBuffercleared periteration +
OverwriteFromNewtonsoftJsonAsync; the per-poll URL interpolation is hoistedto a ctor-built field.
GetMostRecentNotificationsAsyncis deliberately not pooled (itslist escapes to the panel controller).
Contract blast radius: behavior changes only when a registered converter matches the ROOT
target type of an Overwrite call - a combination that today either throws or silently
ignores the converter; all existing callers audited unaffected.
Test
NotificationsRequestControllerShould.ReuseSingleListInstanceAcrossPollIterations-drives two poll iterations against a stubbed web controller; asserts the overwrite op is
used, the same
Targetinstance is passed both iterations, and the buffer is empty atdelivery (the re-dispatch hazard).
GenericDownloadHandlerPopulateIntoShould(3 tests) - converter-routed populate inplace, no duplication after
Clear(), and the no-matching-converter fallback.Validation
Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 1/1 as intended (overwrite op
received 0 calls at pin - the loop allocates a fresh list per poll) / GREEN PASS 4/4.
Side-find from validation (separate issue): a real wire "events_ended" notification would
throw
JsonSerializationExceptionin productionReadJson-NotificationTypehas nomatching enum member.
Fixes #9263
Includes inspection-warning cleanup in all touched files.
Fixes #9263