-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: improve addChild and path performance #1891
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
const components = []; | ||
for (const scope of this.scopes) { | ||
if (scope.node.id) { | ||
components.push(scope.node.id); | ||
} | ||
} | ||
return components.join(Node.PATH_SEP); | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I tested, array access is slightly faster than using the "in" operator |
||
if (this._children[childName]) { | ||
const name = this.id ?? ''; | ||
const typeName = this.host.constructor.name; | ||
throw new Error(`There is already a Construct with name '${childName}' in ${typeName}${name.length > 0 ? ' [' + name + ']' : ''}`); | ||
} | ||
|
||
if (!childName && this.id) { | ||
throw new Error(`cannot add a nameless construct to the named scope: ${this.path}`); | ||
} | ||
Comment on lines
-426
to
-428
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 don't think this if branch can get called. Here's my reasoning:
I tried coming up with a test case that would actually throw this error but I don't think there's a way. |
||
|
||
this._children[childName] = child; | ||
|
||
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'); | ||
} | ||
Comment on lines
-432
to
-434
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 also don't think this branch can ever get called. Here was my reasoning:
|
||
} | ||
} | ||
|
||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling
filter
generates one array, and callingmap
after creates another array. It should be slightly faster to create a single array and perform filtering inside a for loop.