-
Notifications
You must be signed in to change notification settings - Fork 678
fix(projectify): optimize replaceProjectIdToken performance #8358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
43b65d4
8c8ccf2
850f070
fbc4f53
9cacdf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,9 @@ import {Stream} from 'stream'; | |||||||||||||||||||||||||||||||||||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // limitations under the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const PROJECT_ID_TOKEN = '{{projectId}}'; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const PROJECT_ID_TOKEN_REGEX = /{{projectId}}/g; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Populate the `{{projectId}}` placeholder. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -25,33 +28,43 @@ import {Stream} from 'stream'; | |||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||||||||||||||||||||||||||||||||||||||
| export function replaceProjectIdToken(value: any, projectId: string): any { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value = (value as string[]).map(v => replaceProjectIdToken(v, projectId)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof value === 'string') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value.includes(PROJECT_ID_TOKEN)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!projectId || projectId === PROJECT_ID_TOKEN) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new MissingProjectIdError(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value.replace(PROJECT_ID_TOKEN_REGEX, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value === null || typeof value !== 'object') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value !== null && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value === 'object' && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| !(value instanceof Buffer) && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| !(value instanceof Stream) && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value.hasOwnProperty === 'function' | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const opt in value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line no-prototype-builtins | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value.hasOwnProperty(opt)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value[opt] = replaceProjectIdToken(value[opt], projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (let i = 0; i < value.length; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const original = value[i]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const processed = replaceProjectIdToken(original, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (processed !== original) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value[i] = processed; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+45
to
54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutating the input array in-place is a breaking change compared to the original implementation, which used To preserve the performance benefits of avoiding allocations when no placeholders are present, while maintaining safety and backward compatibility, we can use a Copy-on-Write approach. We only clone the array if we actually detect a modified element.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original implementation was already mutating values in-place for all nested objects To address the concern about frozen arrays/objects without triggering new allocations, we implemented a Selective-Write strategy. |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value === 'string' && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (value as string).indexOf('{{projectId}}') > -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!projectId || projectId === '{{projectId}}') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new MissingProjectIdError(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value instanceof Buffer || value instanceof Stream) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we combine this check for Buffer and Stream with the other checks above for null and non-object primitives?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont mind doing this change, but I was simply trying to micro-optimize here. Chances of value having null, object or array is really high compared to Buffer or Stream. So I was trying to avoid this check |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const key in value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Object.prototype.hasOwnProperty.call(value, key)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const original = value[key]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const processed = replaceProjectIdToken(original, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (processed !== original) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be simplified using more modern syntax: for (const key of Object.keys(value)) {
value[key] = replaceProjectIdToken(value[key], projectId);
}I don't think we need the additional
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Earlier, this method was indirectly safe when receiving frozen arrays because the original implementation used .map(), which safely cloned the reference. Since we migrated off .map() to achieve better performance speedup via in-place mutation, we engineered the if (original !== processed) check to preserve that exact historical frozen-structure compatibility. Without it, the unconditional assignment in the Object.keys() snippet would trigger an immediate TypeError in strict mode on frozen structures, even when no replacements are made. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| value[key] = processed; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value = (value as string).replace(/{{projectId}}/g, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
surbhigarg92 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.