Skip to content

perf(spanner): remove rest-parameter allocations in wrappedRequestFn - #9365

Merged
olavloite merged 2 commits into
mainfrom
spanner-remove-rest-param-allocations
Sep 18, 2026
Merged

olavloite merged 2 commits into
mainfrom
spanner-remove-rest-param-allocations

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Every RPC passes through wrappedRequestFn in prepareGapicRequest_. The function previously used rest parameters (...args) and sliced the arguments array on each call, creating two ephemeral arrays per RPC. Because reqOpts and gaxOpts are already bound to requestFn, wrappedRequestFn only receives an optional callback.

This change:

  • Replaces ...args and args.slice with a single optional callback parameter.
  • Reuses the initial promise in non-callback mode instead of invoking requestFn a second time.
  • Preserves .cancel() when requestFn returns a CancellablePromise.
  • Adds try/catch in callback mode to inject the request ID if requestFn throws synchronously.

@olavloite
olavloite requested a review from a team as a code owner September 17, 2026 11:14
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 17, 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 refactors the wrappedRequestFn helper in the Spanner client to robustly inject the Spanner request ID into errors across callbacks, promises, streams, and synchronous exceptions, while preserving the cancel method on cancellable promises. It also adds comprehensive unit and mock server tests to verify these error-handling behaviors. Feedback on the changes suggests adding a truthiness guard on the callback's error argument before passing it to injectRequestIDIntoError to prevent potential runtime exceptions when operations succeed.

Comment thread handwritten/spanner/src/index.ts Outdated
@olavloite
olavloite force-pushed the spanner-remove-rest-param-allocations branch from 7ebedcd to a7657e8 Compare September 17, 2026 11:30
@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 refactors the wrappedRequestFn in prepareGapicRequest_ to decouple error enrichment from the caller's mutable config object by extracting a lightweight errorConfig containing only headers. It also improves error handling across callback, promise, and stream modes to reliably inject the Spanner request ID, and adds comprehensive unit and mock server tests to verify these behaviors. The review feedback suggests reverting the manual backup and restoration of process.env in the tests to Sinon's sandbox.stub to prevent potential test pollution and flakiness.

Comment thread handwritten/spanner/test/index.ts Outdated
@olavloite
olavloite force-pushed the spanner-remove-rest-param-allocations branch from a7657e8 to 0512080 Compare September 17, 2026 11:54
@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 refactors the error-wrapping mechanism in prepareGapicRequest_ to robustly inject Spanner request IDs into errors across callbacks, promises, streams, and synchronous exceptions, while decoupling the configuration and preserving promise cancellation. It also adds comprehensive unit and mock server tests to validate these scenarios. As there are no review comments, we have no additional feedback to provide.

Every RPC passes through wrappedRequestFn in prepareGapicRequest_.
The function previously used rest parameters (...args) and sliced
the arguments array on each call, creating two ephemeral arrays per RPC.
Because reqOpts and gaxOpts are already bound to requestFn,
wrappedRequestFn only receives an optional callback.

This change:
- Replaces ...args and args.slice with a single optional callback parameter.
- Reuses the initial promise in non-callback mode instead of invoking requestFn a second time.
- Preserves .cancel() when requestFn returns a CancellablePromise.
- Adds try/catch in callback mode to inject the request ID if requestFn throws synchronously.
@olavloite
olavloite force-pushed the spanner-remove-rest-param-allocations branch from 0512080 to 156dc83 Compare September 17, 2026 12:07
@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 refactors client instantiation and error handling in the Spanner client, decoupling error enrichment from mutable config objects and ensuring Spanner request IDs are correctly injected across callbacks, promises, and streams. It also adds comprehensive unit and mock tests to verify these behaviors. Feedback suggests reverting the promise chain refactoring from .then().catch() back to .then(onFulfilled, onRejected) to avoid allocating an extra promise, maintaining the PR's focus on performance and reducing allocations.

Comment thread handwritten/spanner/src/index.ts
@olavloite
olavloite requested a review from a team as a code owner September 18, 2026 08:14
@olavloite
olavloite merged commit 7085e03 into main Sep 18, 2026
86 of 87 checks passed
@olavloite
olavloite deleted the spanner-remove-rest-param-allocations branch September 18, 2026 09:05
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.

2 participants