Skip to content
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

Merged
merged 3 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .npmignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .projenrc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,6 @@ project.buildWorkflow?.addPostBuildJobCommands(
{ checkoutRepo: true },
);

project.npmignore?.exclude('/scripts/', '.projenrc.ts');

project.synth();
17 changes: 7 additions & 10 deletions src/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

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 calling map after creates another array. It should be slightly faster to create a single array and perform filtering inside a for loop.

const components = [];
for (const scope of this.scopes) {
if (scope.node.id) {
components.push(scope.node.id);
}
}
return components.join(Node.PATH_SEP);
}

Expand Down Expand Up @@ -417,21 +422,13 @@ export class Node {
throw new Error(`Cannot add children to "${this.path}" during synthesis`);
}

if (childName in this._children) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. addChild is only called if a non-null scope is passed to the Node constructor.
  2. if a non-null scope is passed to the Node constructor, and this.id has a falsey value (like undefined or an empty string), then an error will be thrown in the Node constructor before addChild is called.
  3. based on (1) and (2), when addChild is called, it's guaranteed that childName will not be a falsey value
  4. 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.


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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. From my other comment, childName will never take on a falsey value (like an empty string).
  2. From (1), the line this._children[childName] = childl can never add falsey values as keys to this._children.
  3. There are no other lines of code that add new keys to this._children.
  4. Therefore, this._children will always have keys with truthy values, so Object.keys(this._children).filter(x => !x) can never have a length greater than 0.

}
}

Expand Down
71 changes: 0 additions & 71 deletions test/evaluate-cfn.ts

This file was deleted.