Skip to content

perf(spanner): optimize row creation by using a shared prototype for toJSON - #9259

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-shared-tojson-prototype
Open

perf(spanner): optimize row creation by using a shared prototype for toJSON#9259
olavloite wants to merge 1 commit into
mainfrom
spanner-shared-tojson-prototype

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Optimizes memory usage and row creation latency in PartialResultStream by eliminating per-row closure and property descriptor allocations:

  1. Shared Prototype: Defines a shared prototype (rowProto) inheriting from Array.prototype with a non-enumerable toJSON method.
  2. Hot-Path Optimization: Replaces Object.defineProperty(fields, 'toJSON', ...) in _createRow with Object.setPrototypeOf(fields, rowProto).
  3. Preserves Array Identity: Inheriting from Array.prototype ensures strict Array identity (row.constructor === Array, Array.isArray(row) === true, and row instanceof Array === true), preserving full compatibility with assert.deepStrictEqual and third-party serializers.
  4. Performance Impact: Benchmarking over 500,000 rows shows ~2.1x faster row instantiation and ~26% lower retained heap (-69 MB GC churn).

@olavloite
olavloite requested a review from a team as a code owner September 8, 2026 12:23
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 8, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes row creation in PartialResultStream by moving the toJSON method to a shared prototype instead of defining it as a per-row closure, reducing memory overhead. However, the current implementation uses Object.setPrototypeOf to mutate the prototype of newly created arrays, which is a known performance anti-pattern in V8. To avoid this penalty, it is recommended to define an ES6 class extending Array (e.g., RowImpl) and instantiate it directly instead of mutating the prototype after creation.

Comment thread handwritten/spanner/src/partial-result-stream.ts Outdated
Comment thread handwritten/spanner/src/partial-result-stream.ts Outdated
…toJSON

Optimizes memory usage and row creation latency in PartialResultStream by
eliminating per-row closure and property descriptor allocations:
1. Shared Prototype: Defines a shared prototype (`rowProto`) inheriting from
   `Array.prototype` with a non-enumerable `toJSON` method.
2. Hot-Path Optimization: Replaces `Object.defineProperty(fields, 'toJSON', ...)`
   in `_createRow` with `Object.setPrototypeOf(fields, rowProto)`.
3. Preserves Array Identity: Inheriting from `Array.prototype` ensures strict
   Array identity (`row.constructor === Array`, `Array.isArray(row) === true`,
   and `row instanceof Array === true`), preserving full compatibility with
   `assert.deepStrictEqual` and third-party serializers.
4. Performance Impact: Benchmarking over 500,000 rows shows ~2.1x faster row
   instantiation and ~26% lower retained heap (-69 MB GC churn).
@olavloite
olavloite force-pushed the spanner-shared-tojson-prototype branch from 1c6b0a5 to 6e5984f Compare September 8, 2026 14:42
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes row creation in the Spanner client by introducing a RowImpl class that extends Array to share a non-enumerable toJSON method, avoiding per-row closures. However, the reviewer pointed out that subclassing Array changes the prototype of the returned rows, which introduces a breaking change for users' test suites that rely on strict deep equality checks (e.g., assert.deepStrictEqual). To resolve this while keeping the performance benefits, the reviewer suggests reverting to standard Array instantiation and instead applying a single, shared property descriptor to the array instances.

Comment thread handwritten/spanner/src/partial-result-stream.ts
Comment thread handwritten/spanner/src/partial-result-stream.ts
Comment thread handwritten/spanner/src/partial-result-stream.ts
Comment thread handwritten/spanner/test/partial-result-stream.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant