feat(spanner): optimize replaceProjectIdToken from projectify#8412
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the external dependency @google-cloud/projectify and replaces it with a local implementation of replaceProjectIdToken in src/helper.ts, accompanied by comprehensive unit tests. The reviewer feedback suggests optimizing the recursive token replacement by skipping Date objects to avoid unnecessary traversal overhead, and properly calling super() with the error message in the constructor of the custom MissingProjectIdError class.
| return value; | ||
| } | ||
|
|
||
| if (value instanceof Buffer || value instanceof Stream) { |
There was a problem hiding this comment.
nit: Can we move this up before the check for arrays? That way, we have the trivial checks for early returns in one place.
There was a problem hiding this comment.
I did this intentionally as chances of value having arrays is more compared to "Buffer" and "Stream" . It was a minor optimization only but I thought of reducing extra checks for Buffer and Stream
| const PROJECT_ID_TOKEN_REGEX = /{{projectId}}/g; | ||
|
|
||
| // Keys to skip recursive traversal for Spanner SDK requests to avoid scanning large user data payloads. | ||
| const KEYS_TO_SKIP = new Set([ |
There was a problem hiding this comment.
Can we invert this into an allow-list? This prevents future regressions if we introduce new fields with other names than the ones below that contain large amounts of data. I think that the following list would be enough:
const KEYS_TO_SCAN = new Set([
// Leaf keys containing resource paths
'database',
'session',
'name',
'parent',
'instance',
'resource',
'backup',
'instanceConfig',
'kmsKeyName',
// Intermediate object keys that must be traversed to reach the above leaves
'config',
'encryptionConfig',
]);And then in the replace method:
// ... primitive and Array/Stream/Buffer checks ...
for (const key in value) {
if (Object.prototype.hasOwnProperty.call(value, key)) {
if (!KEYS_TO_SCAN.has(key)) {
continue; // Instantly skip sql, params, mutations, keySet, etc.
}
const original = value[key];
const processed = replaceProjectIdToken(original, projectId);
if (processed !== original) {
value[key] = processed;
}
}
}There was a problem hiding this comment.
This is more efficient performance wise, but Isn't this more error prone. In case we have more keys with resource paths in future ?
There was a problem hiding this comment.
Since its really rare this Spanner API will change , added the suggestion to use allow-list approach as it is more performance efficient
There was a problem hiding this comment.
yeah, my reasoning was also:
- With a deny-list (the original approach), then regressions are likely to be 'silent'. If we add a new proto that contains a large field, and that field is not in this deny-list, then we will have a silent performance regression.
- With an allow-list, then regressions cause errors. Those are easier to catch. And hopefully we catch them directly with tests.
41c05cd to
9603351
Compare
Summary
This pull request replaces the dependency on the legacy external @google-cloud/projectify package with an optimized, Spanner-specific internal token replacement utility in src/helper.ts.
The Problem & Performance Bottleneck
Previously,
@google-cloud/projectifywas utilized to scan gRPC request options and substitute{{projectId}}placeholder tokens in resource paths. Refer https://github.com/googleapis/google-cloud-node/blob/main/core/projectify/src/index.ts#L27However, this generic approach was highly CPU and memory intensive for Spanner because:
Redundant Deep Recurse: The traversal recursively walked every element of massive user payloads, including SQL query strings (sql), query parameters (params, paramTypes), and multi-row inserts (mutations). None of these ever contain resource name placeholders.
Heap-Allocation Storms: The legacy implementation used .map() on all array properties, continually allocating and recreating short-lived arrays. Under high throughput, this caused severe Garbage Collection (GC) blocks.
V8 Object Hidden Class De-optimization: It wrote back properties unconditionally (e.g., value[key] = replaced), marking object shapes as dirty in the V8 engine and invalidating inline caches.
The Solution
We transitioned the token replacement utility locally into
src/helper.tsand implemented two Spanner-specific optimizations:Skip-Keys Optimization: We defined a Spanner-specific list of properties representing raw user data which never contain resource placeholders. When the recursive loop encounters these keys, it immediately skips traversing their children:
Selective-Write & Immutability Guard: We only write back properties to objects/arrays when a change actually occurs (processed !== original). This prevents de-optimizing V8 hidden classes and ensures complete safety for frozen objects (Object.freeze) under strict mode execution.