From a0b37b8aceb2f904563cdcc04efc49b93a617834 Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Fri, 12 Jun 2026 22:03:55 -0400 Subject: [PATCH] perf(core): make texture renderable-owner tracking O(1) on scroll path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During scroll, CoreNode.update sets the IsRenderable bit on nearly every visible node every frame, so updateIsRenderable runs per node per frame and unconditionally called texture.setRenderableOwner. That did an indexOf over renderableOwners: any[] — O(owners) — and SubTexture forwards to the parent atlas texture, whose owner list can hold one entry per sprite/glyph consumer. On a representative trace this was ~165ms of no-op array scans. Two changes: 1. renderableOwners array -> Set. setRenderableOwner now uses has/add/delete with early returns on no-op calls; the 0<->1 size transition logic (renderable flag, onChangeIsRenderable, load()) is unchanged. The four .length consumers move to .size. This is a type-level breaking change for anything introspecting renderableOwners (Inspector, external devtools) — ship as a minor bump. 2. Per-node textureOwnership cache so updateTextureOwnership early-returns when ownership hasn't changed. The cache is synced at every site that touches ownership without going through the helper (unloadTexture, texture setter, CoreTextNode direct call) so a stale true can't skip a re-registration that triggers Texture.load(). After this, steady-state scroll makes zero ownership calls. Adds Texture.test.ts (Set semantics) and CoreNode ownership-cache tests covering the cache skip, out-of-bounds release, texture swap reset, and the freed -> reload re-registration cycle. All 243 unit tests pass. Co-Authored-By: Claude Fable 5 --- src/core/CoreNode.test.ts | 165 ++++++++++++++++++++++++++ src/core/CoreNode.ts | 14 +++ src/core/CoreTextNode.ts | 5 +- src/core/TextureMemoryManager.test.ts | 4 +- src/core/TextureMemoryManager.ts | 2 +- src/core/textures/SubTexture.ts | 2 +- src/core/textures/Texture.test.ts | 124 +++++++++++++++++++ src/core/textures/Texture.ts | 23 ++-- src/main-api/Inspector.ts | 2 +- 9 files changed, 321 insertions(+), 20 deletions(-) create mode 100644 src/core/textures/Texture.test.ts diff --git a/src/core/CoreNode.test.ts b/src/core/CoreNode.test.ts index 6ed1235..167fa5e 100644 --- a/src/core/CoreNode.test.ts +++ b/src/core/CoreNode.test.ts @@ -1712,4 +1712,169 @@ describe('set color()', () => { expect(node.isRenderable).toBe(true); }); }); + + describe('texture ownership cache', () => { + // Same shape as the placeholderColor texture fake: a real EventEmitter so + // loadTextureTask subscribes and we can drive freed/loaded by emitting. + function emittingTexture(state: string): ImageTexture & { + emit: (event: string, data?: unknown) => void; + } { + return Object.assign(new EventEmitter(), { + state, + preventCleanup: false, + retryCount: 0, + maxRetryCount: 1, + dimensions: { w: 100, h: 100 }, + setRenderableOwner: vi.fn(), + }) as unknown as ImageTexture & { + emit: (event: string, data?: unknown) => void; + }; + } + + function visibleNode(): CoreNode { + const parent = new CoreNode(stage, defaultProps()); + parent.globalTransform = Matrix3d.identity(); + parent.worldAlpha = 1; + + const node = new CoreNode(stage, defaultProps({ parent })); + node.alpha = 1; + node.x = 0; + node.y = 0; + node.w = 100; + node.h = 100; + return node; + } + + const flushMicrotasks = () => Promise.resolve(); + + it('repeated updates with unchanged state call setRenderableOwner once', () => { + const node = visibleNode(); + const texture = emittingTexture('loaded'); + node.texture = texture; + node.textureLoaded = true; + + // The texture setter registers ownership with the node's current + // isRenderable (false here), so the cache starts false. + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(1); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + node.update(0, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(2); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + // Steady-state scroll: ownership unchanged, no further calls. + node.update(1, clippingRect); + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes(2); + }); + + it('moving out of bounds releases ownership once, returning re-adds', () => { + const node = visibleNode(); + const texture = emittingTexture('loaded'); + node.texture = texture; + node.textureLoaded = true; + + node.update(0, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + const callsAfterFirstUpdate = ( + texture.setRenderableOwner as ReturnType + ).mock.calls.length; + + // Out of the 200x200 stage bounds entirely. + node.x = 1000; + node.update(1, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 1, + ); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + // Still out of bounds: no repeated release. + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 1, + ); + + // Back in view: re-registered exactly once. + node.x = 0; + node.update(3, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenCalledTimes( + callsAfterFirstUpdate + 2, + ); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + }); + + it('swapping textures releases the old owner and registers the new one', () => { + const node = visibleNode(); + const textureA = emittingTexture('loaded'); + node.texture = textureA; + node.textureLoaded = true; + node.update(0, clippingRect); + expect(textureA.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + const textureB = emittingTexture('loaded'); + node.texture = textureB; + + // A is released via unloadTexture. + expect(textureA.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + // B is registered with the node's current renderable state (true), + // proving the cache reset on swap — a stale cache would skip this. + expect(textureB.setRenderableOwner).toHaveBeenCalledWith( + expect.anything(), + true, + ); + }); + + it('freed texture is re-registered on the next update (reload trigger)', async () => { + const node = visibleNode(); + const texture = emittingTexture('initial'); + node.texture = texture; + node.update(0, clippingRect); + + await flushMicrotasks(); + (texture as { state: string }).state = 'loaded'; + texture.emit('loaded', { w: 100, h: 100 }); + node.update(1, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + + // Memory manager frees the texture: the node must drop ownership... + (texture as { state: string }).state = 'freed'; + texture.emit('freed'); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + false, + ); + + // ...and the next update pass re-adds it, which is what triggers + // Texture.load() for the reload. A stale cache would skip this call. + node.update(2, clippingRect); + expect(texture.setRenderableOwner).toHaveBeenLastCalledWith( + expect.anything(), + true, + ); + }); + }); }); diff --git a/src/core/CoreNode.ts b/src/core/CoreNode.ts index 946a5d1..4d5e2f5 100644 --- a/src/core/CoreNode.ts +++ b/src/core/CoreNode.ts @@ -829,6 +829,14 @@ export class CoreNode extends EventEmitter { private hasColorProps = false; public textureLoaded = false; + /** + * Last ownership value sent to the current texture via + * {@link updateTextureOwnership}. Per (node, texture) pair — must reset to + * `false` whenever the texture is swapped or released, or a stale `true` + * would skip the re-registration that triggers `Texture.load()`. + */ + private textureOwnership = false; + /** * True while this node should render its `placeholderColor` instead of its * texture: `placeholderColor` is non-zero, a texture is set, and that @@ -1085,6 +1093,7 @@ export class CoreNode extends EventEmitter { texture.off('failed', this.onTextureFailed); texture.off('freed', this.onTextureFreed); texture.setRenderableOwner(this._id, false); + this.textureOwnership = false; } protected onTextureLoaded: TextureLoadedEventHandler = (_, dimensions) => { @@ -1918,6 +1927,10 @@ export class CoreNode extends EventEmitter { * Changes the renderable state of the node. */ updateTextureOwnership(isRenderable: boolean) { + if (this.textureOwnership === isRenderable) { + return; + } + this.textureOwnership = isRenderable; this.texture?.setRenderableOwner(this._id, isRenderable); } @@ -3100,6 +3113,7 @@ export class CoreNode extends EventEmitter { this.autosizer.setMode(AutosizeMode.Texture); // Set to texture size mode } value.setRenderableOwner(this._id, this.isRenderable); + this.textureOwnership = this.isRenderable; this.loadTexture(); } diff --git a/src/core/CoreTextNode.ts b/src/core/CoreTextNode.ts index ba3e4c0..4d33db9 100644 --- a/src/core/CoreTextNode.ts +++ b/src/core/CoreTextNode.ts @@ -266,8 +266,9 @@ export class CoreTextNode extends CoreNode implements CoreTextNodeProps { this.setRenderable(false); if (this.renderState > CoreNodeRenderState.OutOfBounds) { - // We do want the texture to load immediately - this.texture.setRenderableOwner(this._id, true); + // We do want the texture to load immediately. Routed through + // updateTextureOwnership so the ownership cache stays in sync. + this.updateTextureOwnership(true); } } } diff --git a/src/core/TextureMemoryManager.test.ts b/src/core/TextureMemoryManager.test.ts index f5692de..72af373 100644 --- a/src/core/TextureMemoryManager.test.ts +++ b/src/core/TextureMemoryManager.test.ts @@ -175,7 +175,7 @@ function freedCachedTexture(cacheKey: string): Texture & { state: 'freed', type: TextureType.image, preventCleanup: false, - renderableOwners: [], + renderableOwners: new Set(), cacheKey, free: vi.fn(), destroy: vi.fn(), @@ -206,7 +206,7 @@ describe('TextureMemoryManager — orphaned freed texture eviction', () => { it('keeps a freed texture that still has renderable owners', () => { const { mgr, keyCache } = makeManager(); const texture = freedCachedTexture('img:poster.png'); - (texture.renderableOwners as unknown[]).push(1); + texture.renderableOwners.add(1); keyCache.set('img:poster.png', texture); mgr.cleanup(); diff --git a/src/core/TextureMemoryManager.ts b/src/core/TextureMemoryManager.ts index 7f95f89..078f3df 100644 --- a/src/core/TextureMemoryManager.ts +++ b/src/core/TextureMemoryManager.ts @@ -349,7 +349,7 @@ export class TextureMemoryManager { if ( evictable === true && texture.preventCleanup === false && - texture.renderableOwners.length === 0 && + texture.renderableOwners.size === 0 && texture.hasListeners() === false ) { this.destroyTexture(texture); diff --git a/src/core/textures/SubTexture.ts b/src/core/textures/SubTexture.ts index ec0ffbf..a5b546a 100644 --- a/src/core/textures/SubTexture.ts +++ b/src/core/textures/SubTexture.ts @@ -79,7 +79,7 @@ export class SubTexture extends Texture { // Resolve parent texture from cache or fallback to provided texture this.parentTexture = txManager.resolveParentTexture(this.props.texture); - if (this.renderableOwners.length > 0) { + if (this.renderableOwners.size > 0) { this.parentTexture.setRenderableOwner(this.subtextureId, true); } diff --git a/src/core/textures/Texture.test.ts b/src/core/textures/Texture.test.ts new file mode 100644 index 0000000..4200ccc --- /dev/null +++ b/src/core/textures/Texture.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect, vi } from 'vitest'; +import { Texture, type TextureData } from './Texture.js'; +import type { CoreTextureManager } from '../CoreTextureManager.js'; + +class TestTexture extends Texture { + override async getTextureSource(): Promise { + return { data: null }; + } +} + +function makeTexture(): { + texture: TestTexture; + loadTexture: ReturnType; + onChangeIsRenderable: ReturnType; +} { + const loadTexture = vi.fn(); + const txManager = { + maxRetryCount: 3, + loadTexture, + } as unknown as CoreTextureManager; + const texture = new TestTexture(txManager); + const onChangeIsRenderable = vi.fn(); + texture.onChangeIsRenderable = onChangeIsRenderable; + return { texture, loadTexture, onChangeIsRenderable }; +} + +describe('Texture.setRenderableOwner', () => { + it('adding an owner fires the 0→1 transition exactly once and loads', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + + expect(texture.renderableOwners.size).toBe(1); + expect(texture.renderable).toBe(true); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(onChangeIsRenderable).toHaveBeenCalledWith(true); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('double-add of the same owner is a no-op (size stays 1, one event)', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, true); + + expect(texture.renderableOwners.size).toBe(1); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('a second distinct owner does not re-fire the transition', () => { + const { texture, loadTexture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(2, true); + + expect(texture.renderableOwners.size).toBe(2); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(1); + expect(loadTexture).toHaveBeenCalledTimes(1); + }); + + it('removing a non-member owner does not fire a transition', () => { + const { texture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, false); + + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + expect(onChangeIsRenderable).not.toHaveBeenCalled(); + }); + + it('1→0 fires onChangeIsRenderable(false) exactly once', () => { + const { texture, onChangeIsRenderable } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, false); + texture.setRenderableOwner(1, false); + + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + expect(onChangeIsRenderable).toHaveBeenCalledTimes(2); + expect(onChangeIsRenderable).toHaveBeenLastCalledWith(false); + }); + + it('re-adding an owner after 1→0 triggers load again (freed→reload cycle)', () => { + const { texture, loadTexture } = makeTexture(); + + texture.setRenderableOwner(1, true); + texture.setRenderableOwner(1, false); + texture.setRenderableOwner(1, true); + + expect(texture.renderable).toBe(true); + expect(loadTexture).toHaveBeenCalledTimes(2); + }); + + it('string and number owners coexist (node ids vs subtexture/font keys)', () => { + const { texture } = makeTexture(); + + texture.setRenderableOwner(7, true); + texture.setRenderableOwner('subtexture-7', true); + + expect(texture.renderableOwners.size).toBe(2); + + texture.setRenderableOwner(7, false); + expect(texture.renderableOwners.size).toBe(1); + expect(texture.renderable).toBe(true); + + texture.setRenderableOwner('subtexture-7', false); + expect(texture.renderableOwners.size).toBe(0); + expect(texture.renderable).toBe(false); + }); + + it('canBeCleanedUp respects remaining owners', () => { + const { texture } = makeTexture(); + // Skip the startup grace period so owners are the deciding factor. + texture.isWithinStartupGracePeriod = () => false; + + texture.setRenderableOwner(1, true); + expect(texture.canBeCleanedUp()).toBe(false); + + texture.setRenderableOwner(1, false); + expect(texture.canBeCleanedUp()).toBe(true); + }); +}); diff --git a/src/core/textures/Texture.ts b/src/core/textures/Texture.ts index dcfd5f7..3f85b0f 100644 --- a/src/core/textures/Texture.ts +++ b/src/core/textures/Texture.ts @@ -141,7 +141,7 @@ export abstract class Texture extends EventEmitter { // aggregate state public state: TextureState = 'initial'; - readonly renderableOwners: any[] = []; + readonly renderableOwners: Set = new Set(); readonly renderable: boolean = false; @@ -251,7 +251,7 @@ export abstract class Texture extends EventEmitter { } // Don't cleanup if there are still renderable owners - if (this.renderableOwners.length > 0) { + if (this.renderableOwners.size > 0) { return false; } @@ -274,28 +274,25 @@ export abstract class Texture extends EventEmitter { * @param renderable */ setRenderableOwner(owner: string | number, renderable: boolean): void { - const oldSize = this.renderableOwners.length; - const hasOwnerIndex = this.renderableOwners.indexOf(owner); + const owners = this.renderableOwners; if (renderable === true) { - if (hasOwnerIndex === -1) { - // Add the owner to the set - this.renderableOwners.push(owner); + if (owners.has(owner) === true) { + return; } - const newSize = this.renderableOwners.length; - if (oldSize !== newSize && newSize === 1) { + owners.add(owner); + if (owners.size === 1) { (this.renderable as boolean) = true; this.onChangeIsRenderable?.(true); this.load(); } } else { - if (hasOwnerIndex !== -1) { - this.renderableOwners.splice(hasOwnerIndex, 1); + if (owners.delete(owner) === false) { + return; } - const newSize = this.renderableOwners.length; - if (oldSize !== newSize && newSize === 0) { + if (owners.size === 0) { (this.renderable as boolean) = false; this.onChangeIsRenderable?.(false); diff --git a/src/main-api/Inspector.ts b/src/main-api/Inspector.ts index 652915a..b8aae5a 100644 --- a/src/main-api/Inspector.ts +++ b/src/main-api/Inspector.ts @@ -1104,7 +1104,7 @@ export class Inspector { // Update renderable owners count div.setAttribute( 'data-texture-owners', - String(texture.renderableOwners.length), + String(texture.renderableOwners.size), ); // Update retry count