From 99e30b69cc04e7ddb8edaab6b4db363ba5ae1bed Mon Sep 17 00:00:00 2001 From: Marcelo Lv Cabral Date: Tue, 28 Jul 2026 15:47:05 -0700 Subject: [PATCH] fix(scenegraph): cross script-scope m as live references, only on owner transfer Two device-confirmed corrections to how a node's script-scope `m` crosses threads, both measured with an instrumented app run on hardware. `m` holds a LIVE node reference. Mutating a node stored in `m` from the receiving thread is visible through `m` on a device; here it was a detached snapshot that silently stopped tracking. Node-valued entries now cross as an address and resolve to this thread's own instance, live-reachability first with the cross-thread registry as fallback (the ordering Task.resolveNode documents). An address with no local instance is dropped with a one-shot warning rather than restored as a stale copy. That also removes the dominant cost. A component caching m.scene / m.parentScreen dragged the whole scene tree into the payload: measured 44.4 KB against 0.2 KB for the same node with scalar-only `m`, a 230x blowup paid on every serialization. It is now 0.3 KB. `m` also only needs to travel when the node changes owner. A callFunc on a node owned by another thread rendezvouses to that owner and runs against the owner's `m` (device-confirmed), so the copy shipped everywhere else was never read. `_m_` is now opt-in and enabled only at the task -> render ownership transfer, removing it from callFunc args, rendezvous responses, render-thread-queue posts and nodes nested in associative arrays. Not covered: a node reached through an array or associative array inside `m` still serializes as a copy. Co-Authored-By: Claude Opus 5 --- src/extensions/scenegraph/SGRoot.ts | 29 +++++++ .../scenegraph/factory/Serializer.ts | 83 +++++++++++++++---- src/extensions/scenegraph/nodes/Task.ts | 9 +- .../scenegraph/NodeSerialization.test.js | 64 ++++++++++++-- 4 files changed, 161 insertions(+), 24 deletions(-) diff --git a/src/extensions/scenegraph/SGRoot.ts b/src/extensions/scenegraph/SGRoot.ts index 5a69ade4f..7a8f62552 100644 --- a/src/extensions/scenegraph/SGRoot.ts +++ b/src/extensions/scenegraph/SGRoot.ts @@ -376,6 +376,35 @@ export class SGRoot { return this._crossThreadNodes.get(address)?.deref(); } + /** + * Resolves an address to the instance of that node this thread should actually use. + * + * Live reachability wins over the cross-thread registry: `toSGNode` copies `_address_` when it + * rebuilds a node, so repeated serializations of one logical node mint instances sharing its + * address and the registry keeps only the latest. Walking the trees first means a node still + * wired into the scene, the global node or a task always resolves to its authoritative copy, + * with the registry left as the fallback for a true orphan held only by the other thread. + * Mirrors the ordering in `Task.resolveNode` — see the regression it documents. + * @param address Node address to resolve. + * @returns The live node, or undefined when this thread has no instance for that address. + */ + resolveLiveNode(address: string): Node | undefined { + if (!address) { + return undefined; + } + const roots: (Node | undefined)[] = [this._scene, this._mGlobal]; + for (const thread of this._threads.values()) { + roots.push(thread.task); + } + for (const root of roots) { + const found = root?.findNodeByAddress(root, address, true); + if (found) { + return found; + } + } + return this.getCrossThreadNode(address); + } + /** * Adds a new task thread to the SGRoot. * @param task Task instance to add diff --git a/src/extensions/scenegraph/factory/Serializer.ts b/src/extensions/scenegraph/factory/Serializer.ts index a7d6f19b2..c5f248933 100644 --- a/src/extensions/scenegraph/factory/Serializer.ts +++ b/src/extensions/scenegraph/factory/Serializer.ts @@ -529,6 +529,47 @@ export function toSGNode(obj: any, type: string, subtype: string, child?: boolea return newNode; } +/** + * Serializes a custom component's script-scope `m` for a node that is changing owner. + * + * Node-valued entries are emitted as address references, not copies. Device-confirmed: `m` holds a + * *live* reference — mutating the referenced node on the receiving thread is visible through `m` — + * so a deep copy is both wrong (a detached snapshot that silently stops tracking) and expensive: a + * component caching `m.scene`/`m.parentScreen` dragged the whole scene tree into every payload, + * measured at 44 KB against 0.2 KB for the same node with scalar-only `m`. + * @param node Node whose `m` is being serialized. + * @param host Optional host node for observing context. + * @param visited Visited set of the enclosing pass. + * @returns The serialized `m` entries, excluding `top`/`global`. + */ +function serializeScriptScopeM(node: Node, host: Node | undefined, visited: WeakSet): FlexObject { + const mData: FlexObject = {}; + // Scoped so the values serialized here don't stay marked as visited: the enclosing pass may + // reach the same values by another route and must serialize them in full (see ScopedVisited). + const scoped = new ScopedVisited(visited); + const scopedVisited = scoped as unknown as WeakSet; + try { + for (const [key, value] of node.m.elements) { + const lowerKey = key.toLowerCase(); + if (lowerKey === "top" || lowerKey === "global") { + continue; + } + if (value instanceof Node) { + sgRoot.registerCrossThreadNode(value); + mData[key] = { _mref_: value.getAddress() }; + continue; + } + mData[key] = jsValueOf(value, true, host, scopedVisited); + } + } finally { + scoped.rollback(); + } + return mData; +} + +/** One-shot warning when a script-scope node reference has no instance on the receiving thread. */ +let warnedUnresolvedScriptScopeRef = false; + /** * Merges a serialized script-scope `m` (see fromSGNode's `_m_` entry) into a node's `m`, * preserving the locally built `top`/`global` entries. No-op for payloads without `_m_`. @@ -541,6 +582,20 @@ function restoreScriptScopeM(serializedM: any, node: Node, nodeMap?: Map): FlexObject { +export function fromSGNode( + node: Node, + deep: boolean = true, + host?: Node, + visited?: WeakSet, + scriptScope: boolean = false +): FlexObject { visited ??= new WeakSet(); if (visited.has(node)) { return { @@ -770,7 +831,7 @@ export function fromSGNode(node: Node, deep: boolean = true, host?: Node, visite } } if (fieldValue instanceof Node) { - result[name] = fromSGNode(fieldValue, deep, host, visited); + result[name] = fromSGNode(fieldValue, deep, host, visited, scriptScope); continue; } const serialized = jsValueOf(fieldValue, deep, host, visited); @@ -793,20 +854,8 @@ export function fromSGNode(node: Node, deep: boolean = true, host?: Node, visite // callFunc there would otherwise read init()-set variables back as invalid. `top`/`global` // are excluded — the receiver rebuilds them locally, and serializing them here would re-walk // the node and ship the entire global tree. - if (deep && sgRoot.nodeDefMap.has(node.nodeSubtype.toLowerCase())) { - const mData: FlexObject = {}; - // Scoped so the values serialized here don't stay marked as visited: the enclosing pass may - // reach the same values by another route and must serialize them in full (see ScopedVisited). - const scoped = new ScopedVisited(visited); - const scopedVisited = scoped as unknown as WeakSet; - for (const [key, value] of node.m.elements) { - const lowerKey = key.toLowerCase(); - if (lowerKey === "top" || lowerKey === "global") { - continue; - } - mData[key] = jsValueOf(value, deep, host, scopedVisited); - } - scoped.rollback(); + if (deep && scriptScope && sgRoot.nodeDefMap.has(node.nodeSubtype.toLowerCase())) { + const mData = serializeScriptScopeM(node, host, visited); if (Object.keys(mData).length) { result["_m_"] = mData; } @@ -815,7 +864,7 @@ export function fromSGNode(node: Node, deep: boolean = true, host?: Node, visite if (deep && children.length > 0 && node.serializesChildren()) { result["_children_"] = children.map((child: BrsType) => { if (child instanceof Node) { - return fromSGNode(child, deep, host, visited); + return fromSGNode(child, deep, host, visited, scriptScope); } return { _invalid_: null }; }); diff --git a/src/extensions/scenegraph/nodes/Task.ts b/src/extensions/scenegraph/nodes/Task.ts index 8fead441b..770c11a77 100644 --- a/src/extensions/scenegraph/nodes/Task.ts +++ b/src/extensions/scenegraph/nodes/Task.ts @@ -381,7 +381,14 @@ export class Task extends Node { if (this.threadId < 0 || !this.active) { return; } - const value = fieldValue instanceof Node ? fromSGNode(fieldValue, true) : jsValueOf(fieldValue); + // A node crossing task → render changes owner (below), so the receiver runs callFunc against + // its own copy and needs the script-scope `m` that init() populated here. Every other path + // leaves the owner unchanged, and a callFunc on a node owned elsewhere rendezvouses to that + // owner (device-confirmed), so shipping `m` there would only bloat the payload. + const value = + fieldValue instanceof Node + ? fromSGNode(fieldValue, true, undefined, undefined, this.inThread) + : jsValueOf(fieldValue); if (fieldValue instanceof Node) { // Re-own to the render thread only when the node actually crosses task → render (a task // setting a field). On the render side this is a fan-out (render → task): the node stays diff --git a/test/extensions/scenegraph/NodeSerialization.test.js b/test/extensions/scenegraph/NodeSerialization.test.js index f5db959c8..490066684 100644 --- a/test/extensions/scenegraph/NodeSerialization.test.js +++ b/test/extensions/scenegraph/NodeSerialization.test.js @@ -2,7 +2,7 @@ const scenegraph = require("../../../packages/scenegraph/lib/brs-sg.node.js"); const core = require("../../../packages/node/bin/brs.node.js"); const { Node, fromSGNode, toSGNode, updateSGNode, jsValueOf, fromAssociativeArray } = scenegraph; -const { ComponentDefinition, sgRoot, createFlatNode } = scenegraph; +const { ComponentDefinition, sgRoot, createFlatNode, SGNodeFactory } = scenegraph; const { BrsInvalid, isInvalid, BrsString, BrsBoolean, RoAssociativeArray, RoArray } = core; /** Simulates the structured/JSON round-trip a node undergoes when sent to a Task thread. */ @@ -116,6 +116,9 @@ describe("SceneGraph node serialization", () => { // Mirrors the device contract: a node created on a Task thread runs init() there // (populating its script-scope `m`), and a later callFunc on the receiving thread must // still see that state — init() is never re-run on the other side. + // + // `m` only travels on the ownership-transfer path, so these serialize with scriptScope on. + const withScope = (node, deep = true) => fromSGNode(node, deep, undefined, undefined, true); beforeEach(() => { const def = new ComponentDefinition("pkg:/components/CustomHelper.xml"); def.name = "CustomHelper"; @@ -142,12 +145,12 @@ describe("SceneGraph node serialization", () => { } test("serializes init()-set m entries, excluding top and global", () => { - const serialized = fromSGNode(makeInitializedNode(), true); + const serialized = withScope(makeInitializedNode()); expect(serialized._m_).toEqual({ setupCalled: false, label: "ready" }); }); test("restores m on the receiving thread so callFunc-visible state survives", () => { - const serialized = fromSGNode(makeInitializedNode(), true); + const serialized = withScope(makeInitializedNode()); const target = toSGNode(transfer(serialized), "Node", "CustomHelper"); expect(mValue(target, "setupCalled")).toBe(false); expect(mValue(target, "label")).toBe("ready"); @@ -170,20 +173,69 @@ describe("SceneGraph node serialization", () => { const serialized = fromAssociativeArray(m, true, taskNode); expect(serialized.mynode._circular_).toBeUndefined(); expect(serialized.mynode.title).toBe("hello"); + + // Same guarantee on the transfer path, where `_m_` is actually emitted. + const withM = fromSGNode(taskNode, true, taskNode, undefined, true); + expect(withM._m_.mynode._mref_).toBe(content.getAddress()); }); test("a built-in node never emits _m_", () => { const node = new Node([], "Node"); node.m.set(new BrsString("stuff"), new BrsString("internal")); - expect(fromSGNode(node, true)._m_).toBeUndefined(); + expect(withScope(node)._m_).toBeUndefined(); + }); + + test("emits _m_ only for an ownership transfer, not for an ordinary serialization", () => { + // A node keeping its owner is read back through a rendezvous to that owner, so its `m` + // never travels — shipping it would only bloat every field write and callFunc arg. + expect(fromSGNode(makeInitializedNode(), true)._m_).toBeUndefined(); + }); + + test("stores a node in m as a live reference, not a copy of its subtree", () => { + const scene = SGNodeFactory.createNode("Scene"); + const shared = new Node([], "ContentNode"); + shared.setValue("id", new BrsString("SharedContent"), false); + shared.setValueSilent("title", new BrsString("set-in-task")); + scene.appendChildToParent(shared); + sgRoot.setScene(scene); + + const node = makeInitializedNode(); + node.m.set(new BrsString("stashed"), shared, true); + + // Device-confirmed: `m` holds a live reference. It crosses as an address, not a subtree. + const serialized = withScope(node); + expect(serialized._m_.stashed).toEqual({ _mref_: shared.getAddress() }); + + // On the receiving side it resolves back to this thread's own instance, so a later + // mutation of that node is visible through `m` — a copy would have frozen the value. + const target = toSGNode(transfer(serialized), "Node", "CustomHelper"); + const restored = target.m.get(new BrsString("stashed")); + expect(restored).toBe(shared); + shared.setValueSilent("title", new BrsString("changed-on-render")); + expect(jsValueOf(restored.getValue("title"))).toBe("changed-on-render"); + }); + + test("drops a script-scope reference whose node has not crossed", () => { + sgRoot.setScene(SGNodeFactory.createNode("Scene")); + const node = makeInitializedNode(); + const orphan = new Node([], "ContentNode"); + node.m.set(new BrsString("stashed"), orphan, true); + + const serialized = transfer(withScope(node)); + // Force an address this thread cannot resolve to any live instance. + serialized._m_.stashed._mref_ = "DEADBEEF000000"; + const target = toSGNode(serialized, "Node", "CustomHelper"); + expect(isInvalid(target.m.get(new BrsString("stashed")))).toBe(true); + // The rest of `m` still restores. + expect(mValue(target, "label")).toBe("ready"); }); test("a shallow serialization (deep = false) skips m", () => { - expect(fromSGNode(makeInitializedNode(), false)._m_).toBeUndefined(); + expect(withScope(makeInitializedNode(), false)._m_).toBeUndefined(); }); test("updateSGNode populates m on a flat node but never clobbers local state", () => { - const serialized = transfer(fromSGNode(makeInitializedNode(), true)); + const serialized = transfer(withScope(makeInitializedNode())); const flat = createFlatNode("Node", "CustomHelper"); updateSGNode(serialized, flat);