diff --git a/.changeset/eight-lines-dig.md b/.changeset/eight-lines-dig.md new file mode 100644 index 000000000000..ea8077574125 --- /dev/null +++ b/.changeset/eight-lines-dig.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixed an issue where queued rendering wasn't correctly re-using the saved nodes. diff --git a/packages/astro/src/runtime/server/render/queue/pool.ts b/packages/astro/src/runtime/server/render/queue/pool.ts index c161beb94d2d..3af10862ee28 100644 --- a/packages/astro/src/runtime/server/render/queue/pool.ts +++ b/packages/astro/src/runtime/server/render/queue/pool.ts @@ -1,4 +1,10 @@ -import type { QueueNode } from './types.js'; +import type { + QueueNode, + TextNode, + HtmlStringNode, + ComponentNode, + InstructionNode, +} from './types.js'; import type { SSRManifest } from '../../../../core/app/types.js'; import { queueContentCache, queuePoolSize } from '../../../../core/app/manifest.js'; @@ -41,7 +47,10 @@ export interface PoolStatsReport extends PoolStats { * This significantly reduces memory allocation overhead when building large queues. */ export class NodePool { - private pool: QueueNode[] = []; + private textPool: TextNode[] = []; + private htmlStringPool: HtmlStringNode[] = []; + private componentPool: ComponentNode[] = []; + private instructionPool: InstructionNode[] = []; private contentCache = new Map(); public readonly maxSize: number; private readonly enableStats: boolean; @@ -110,22 +119,24 @@ export class NodePool { return this.cloneNode(template); } - // Standard pooling (no content caching) - const pooledNode = this.pool.pop(); + // Standard pooling - pop from the type-specific sub-pool and reuse the object + const pooledNode = this.popFromTypedPool(type); if (pooledNode) { if (this.enableStats) { this.stats.acquireFromPool = this.stats.acquireFromPool + 1; } - return this.createNode(type, ''); + // Reassign value field on the reused object (type discriminant is already correct) + this.resetNodeContent(pooledNode, type, content); + return pooledNode; } - // Pool is empty, create new node + // Pool is empty for this type, create new node if (this.enableStats) { this.stats.acquireNew = this.stats.acquireNew + 1; } - return this.createNode(type, ''); + return this.createNode(type, content); } /** @@ -162,6 +173,56 @@ export class NodePool { } } + /** + * Pops a node from the type-specific sub-pool. + * Returns undefined if the sub-pool for the requested type is empty. + */ + private popFromTypedPool(type: QueueNode['type']): QueueNode | undefined { + switch (type) { + case 'text': + return this.textPool.pop(); + case 'html-string': + return this.htmlStringPool.pop(); + case 'component': + return this.componentPool.pop(); + case 'instruction': + return this.instructionPool.pop(); + } + } + + /** + * Resets the content/value field on a reused pooled node. + * The type discriminant is already correct since we pop from the matching sub-pool. + */ + private resetNodeContent(node: QueueNode, type: QueueNode['type'], content?: string): void { + switch (type) { + case 'text': + (node as TextNode).content = content ?? ''; + break; + case 'html-string': + (node as HtmlStringNode).html = content ?? ''; + break; + case 'component': + (node as ComponentNode).instance = undefined as any; + break; + case 'instruction': + (node as InstructionNode).instruction = undefined as any; + break; + } + } + + /** + * Returns the total number of nodes across all typed sub-pools. + */ + private totalPoolSize(): number { + return ( + this.textPool.length + + this.htmlStringPool.length + + this.componentPool.length + + this.instructionPool.length + ); + } + /** * Releases a queue node back to the pool for reuse. * If the pool is at max capacity, the node is discarded (will be GC'd). @@ -169,17 +230,38 @@ export class NodePool { * @param node - The node to release back to the pool */ release(node: QueueNode): void { - if (this.pool.length < this.maxSize) { - this.pool.push(node); - if (this.enableStats) { - this.stats.released = this.stats.released + 1; - } - } else { + if (this.totalPoolSize() >= this.maxSize) { if (this.enableStats) { this.stats.releasedDropped = this.stats.releasedDropped + 1; } + // Pool is full, let the node be garbage collected + return; + } + + // Route to the correct typed sub-pool and clear value fields + // to avoid retaining references across renders + switch (node.type) { + case 'text': + node.content = ''; + this.textPool.push(node); + break; + case 'html-string': + node.html = ''; + this.htmlStringPool.push(node); + break; + case 'component': + node.instance = undefined as any; + this.componentPool.push(node); + break; + case 'instruction': + node.instruction = undefined as any; + this.instructionPool.push(node); + break; + } + + if (this.enableStats) { + this.stats.released = this.stats.released + 1; } - // If the pool is full, let the node be garbage collected } /** @@ -195,11 +277,14 @@ export class NodePool { } /** - * Clears the pool, discarding all cached nodes. + * Clears all typed sub-pools, discarding all cached nodes. * This can be useful if you want to free memory after a large render. */ clear(): void { - this.pool.length = 0; + this.textPool.length = 0; + this.htmlStringPool.length = 0; + this.componentPool.length = 0; + this.instructionPool.length = 0; } /** @@ -225,13 +310,13 @@ export class NodePool { } /** - * Gets the current number of nodes in the pool. + * Gets the current total number of nodes across all typed sub-pools. * Useful for monitoring pool usage and tuning maxSize. * * @returns Number of nodes currently available in the pool */ size(): number { - return this.pool.length; + return this.totalPoolSize(); } /** @@ -242,7 +327,7 @@ export class NodePool { getStats(): PoolStatsReport { return { ...this.stats, - poolSize: this.pool.length, + poolSize: this.totalPoolSize(), maxSize: this.maxSize, hitRate: this.stats.acquireFromPool + this.stats.acquireNew > 0 diff --git a/packages/astro/test/units/render/queue-pool.test.js b/packages/astro/test/units/render/queue-pool.test.js index d811a7103dd1..a0b9a6aed168 100644 --- a/packages/astro/test/units/render/queue-pool.test.js +++ b/packages/astro/test/units/render/queue-pool.test.js @@ -1,5 +1,5 @@ import { describe, it } from 'node:test'; -import { strictEqual } from 'node:assert'; +import { strictEqual, notStrictEqual } from 'node:assert'; import { NodePool } from '../../../dist/runtime/server/render/queue/pool.js'; describe('NodePool', () => { @@ -11,10 +11,10 @@ describe('NodePool', () => { strictEqual(node.content, ''); // Default value for new TextNode }); - it('should reuse released nodes', () => { + it('should reuse released nodes of the same type', () => { const pool = new NodePool(); - // Acquire and set up a node + // Acquire and set up a text node const node1 = pool.acquire('text'); node1.content = 'Hello'; @@ -22,15 +22,34 @@ describe('NodePool', () => { pool.release(node1); strictEqual(pool.size(), 1); - // Acquire another node - with discriminated union, we create a fresh node - const node2 = pool.acquire('html-string'); - strictEqual(node2.type, 'html-string'); // Type is html-string - strictEqual(node2.html, ''); // Default value for new HtmlStringNode + // Acquire another text node - should reuse the same object + const node2 = pool.acquire('text'); + strictEqual(node2.type, 'text'); + strictEqual(node2.content, ''); // Content was reset on release + strictEqual(node1, node2); // Same object reference (actual reuse) - // Pool size should decrease (node was consumed from pool) + // Pool size should decrease (node was consumed from the text sub-pool) strictEqual(pool.size(), 0); }); + it('should not reuse released nodes across different types', () => { + const pool = new NodePool(); + + // Acquire and release a text node + const node1 = pool.acquire('text'); + node1.content = 'Hello'; + pool.release(node1); + strictEqual(pool.size(), 1); + + // Acquire an html-string node - should NOT reuse the text node + const node2 = pool.acquire('html-string'); + strictEqual(node2.type, 'html-string'); + strictEqual(node2.html, ''); + + // Text node still in the text sub-pool (html-string pool was empty) + strictEqual(pool.size(), 1); + }); + it('should respect maxSize limit', () => { const pool = new NodePool(2); // Max size of 2 @@ -98,7 +117,7 @@ describe('NodePool', () => { it('should handle multiple acquire/release cycles', () => { const pool = new NodePool(10); - // First cycle + // First cycle - acquire and release text nodes const batch1 = []; for (let i = 0; i < 5; i++) { batch1.push(pool.acquire('text')); @@ -106,12 +125,12 @@ describe('NodePool', () => { pool.releaseAll(batch1); strictEqual(pool.size(), 5); - // Second cycle - should reuse from pool + // Second cycle - reuse from the same type (text) sub-pool const batch2 = []; for (let i = 0; i < 3; i++) { - batch2.push(pool.acquire('html-string')); + batch2.push(pool.acquire('text')); } - strictEqual(pool.size(), 2); // 5 - 3 = 2 remaining + strictEqual(pool.size(), 2); // 5 - 3 = 2 remaining in text pool pool.releaseAll(batch2); strictEqual(pool.size(), 5); // 2 + 3 = 5 @@ -135,4 +154,122 @@ describe('NodePool', () => { } strictEqual(pool.size(), 0); // All reused }); + + it('should return the same object reference when reusing pooled nodes', () => { + const pool = new NodePool(); + + // Test all four node types for identity reuse + const types = ['text', 'html-string', 'component', 'instruction']; + + for (const type of types) { + const original = pool.acquire(type); + pool.release(original); + const reused = pool.acquire(type); + strictEqual(original, reused, `${type} node should be same object after reuse`); + } + }); + + it('should clear references on component and instruction nodes when released', () => { + const pool = new NodePool(); + + // Component node - instance should be cleared + const compNode = pool.acquire('component'); + compNode.instance = { render: () => {} }; // Simulate a component instance + pool.release(compNode); + + const reusedComp = pool.acquire('component'); + strictEqual(reusedComp, compNode); // Same object + strictEqual(reusedComp.instance, undefined); // Instance cleared on release + + // Instruction node - instruction should be cleared + const instrNode = pool.acquire('instruction'); + instrNode.instruction = { type: 'head' }; // Simulate an instruction + pool.release(instrNode); + + const reusedInstr = pool.acquire('instruction'); + strictEqual(reusedInstr, instrNode); // Same object + strictEqual(reusedInstr.instruction, undefined); // Instruction cleared on release + }); + + it('should track pool size across mixed types correctly', () => { + const pool = new NodePool(10); + + // Release nodes of different types + const text1 = pool.acquire('text'); + const text2 = pool.acquire('text'); + const html1 = pool.acquire('html-string'); + const comp1 = pool.acquire('component'); + const instr1 = pool.acquire('instruction'); + + pool.releaseAll([text1, text2, html1, comp1, instr1]); + strictEqual(pool.size(), 5); // Total across all sub-pools + + // Acquire from specific types - only those sub-pools decrease + pool.acquire('text'); + strictEqual(pool.size(), 4); + + pool.acquire('text'); + strictEqual(pool.size(), 3); + + // Text sub-pool is now empty; acquiring another text creates new (no change to pool size) + const newText = pool.acquire('text'); + strictEqual(pool.size(), 3); // Still 3 (html, component, instruction remain) + notStrictEqual(newText, text1); // Not reused - new object + notStrictEqual(newText, text2); // Not reused - new object + }); + + it('should apply shared maxSize cap across all sub-pools', () => { + const pool = new NodePool(3); // Max 3 total across all types + + const text1 = pool.acquire('text'); + const html1 = pool.acquire('html-string'); + const comp1 = pool.acquire('component'); + const instr1 = pool.acquire('instruction'); + + pool.release(text1); // 1/3 + pool.release(html1); // 2/3 + pool.release(comp1); // 3/3 + pool.release(instr1); // Exceeds cap - dropped + + strictEqual(pool.size(), 3); + + // The instruction node was dropped, so acquiring instruction creates new + const newInstr = pool.acquire('instruction'); + notStrictEqual(newInstr, instr1); + }); + + it('should set content on reused nodes via acquire', () => { + const pool = new NodePool(); + + // Release a text node + const node = pool.acquire('text'); + node.content = 'old content'; + pool.release(node); + + // Acquire with content parameter - content should be set on the reused node + const reused = pool.acquire('text', 'new content'); + strictEqual(reused, node); // Same object + strictEqual(reused.content, 'new content'); + }); + + it('should clear all sub-pools on clear()', () => { + const pool = new NodePool(); + + // Release one of each type + const nodes = [ + pool.acquire('text'), + pool.acquire('html-string'), + pool.acquire('component'), + pool.acquire('instruction'), + ]; + pool.releaseAll(nodes); + strictEqual(pool.size(), 4); + + pool.clear(); + strictEqual(pool.size(), 0); + + // Acquiring after clear should create new objects (not reuse) + const newText = pool.acquire('text'); + notStrictEqual(newText, nodes[0]); + }); });