fix: improve addChild and path performance #1891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Chriscbr
commented
Aug 27, 2023
| @@ -77,7 +77,12 @@ export class Node { | |||
| * Components are separated by '/'. | |||
| */ | |||
| public get path(): string { | |||
| const components = this.scopes.filter(c => c.node.id).map(c => c.node.id); | |||
There was a problem hiding this comment.
Calling filter generates one array, and calling map after creates another array. It should be slightly faster to create a single array and perform filtering inside a for loop.
| @@ -417,21 +422,13 @@ export class Node { | |||
| throw new Error(`Cannot add children to "${this.path}" during synthesis`); | |||
| } | |||
|
|
|||
| if (childName in this._children) { | |||
There was a problem hiding this comment.
From what I tested, array access is slightly faster than using the "in" operator
Comment on lines
-426
to
-428
| if (!childName && this.id) { | ||
| throw new Error(`cannot add a nameless construct to the named scope: ${this.path}`); | ||
| } |
There was a problem hiding this comment.
I don't think this if branch can get called. Here's my reasoning:
addChildis only called if a non-nullscopeis passed to theNodeconstructor.- if a non-null
scopeis passed to theNodeconstructor, andthis.idhas a falsey value (likeundefinedor an empty string), then an error will be thrown in theNodeconstructor beforeaddChildis called. - based on (1) and (2), when
addChildis called, it's guaranteed thatchildNamewill not be a falsey value - the if statement's body only executes if childName takes on a falsey value
I tried coming up with a test case that would actually throw this error but I don't think there's a way.
Comment on lines
-432
to
-434
| if (Object.keys(this._children).length > 1 && Object.keys(this._children).filter(x => !x).length > 0) { | ||
| throw new Error('only a single construct is allowed in a scope if it has an empty name'); | ||
| } |
There was a problem hiding this comment.
I also don't think this branch can ever get called. Here was my reasoning:
- From my other comment,
childNamewill never take on a falsey value (like an empty string). - From (1), the line
this._children[childName] = childlcan never add falsey values as keys tothis._children. - There are no other lines of code that add new keys to
this._children. - Therefore,
this._childrenwill always have keys with truthy values, soObject.keys(this._children).filter(x => !x)can never have a length greater than 0.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improve the performance of the
constructslibrary by removing several branches in the library's hot paths and switching to some more performant operations.On my laptop, the benchmark I used (shared in a gist here) takes about 5.0-5.3 seconds with the old version of constructs, and about 1.1-1.2 seconds with the new version of constructs.
Misc:
testfolder.npmignoreto reduce the package's npm footprint.