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.

Sign up for GitHub

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

Jump to bottom

Performance bug in angular/packages/compiler/src/output/output_ast.ts #31627

Closed
tebbi opened this issue Jul 18, 2019 · 3 comments
Closed

Performance bug in angular/packages/compiler/src/output/output_ast.ts #31627

tebbi opened this issue Jul 18, 2019 · 3 comments
Labels
area: compiler Issues related to `ngc`, Angular's template compiler freq3: high type: bug/fix
Milestone
Backlog

Comments

@tebbi
Copy link

tebbi commented Jul 18, 2019

angular/packages/compiler/src/output/output_ast.ts

Lines 1465 to 1471 in e8ae3c5

private _clone(obj: any): any {
const clone = Object.create(obj.constructor.prototype);
for (let prop in obj) {
clone[prop] = obj[prop];
}
return clone;
}

This for-in loop also iterates over the prototype chain, copying many functions from the prototypes onto the cloned object as own properties. This doesn't make sense, since the cloned object also has the right prototype, so in a way it has all these functions twice now: on the prototype chain and as own properties.
Putting a check in this loop and only copying own properties will improve performance and avoid wasting memory on unnecessary properties.

In V8 versions before 7.4 (Chrome 74, Node 12), we had an optimization that improved the situation. This is why Node.js observed a huge memory regression for Angular builds when updating V8 in Node.js 12: nodejs/node#28205
That is, we had an optimization that detected function properties that are always the same and shares them in the hidden class instead of actually putting them on the object. This is kind of V8 doing something like prototypes without the user using prototypes. When we enabled constant field tracking, which serves a similar purpose in other cases, we removed this old optimization. This results in us always putting own properties into the object, so they always consume memory, as one might expect actually.

Is this code only used for Angular builds or does it also affect webpages?
@AndrewKushnir AndrewKushnir added the area: compiler Issues related to `ngc`, Angular's template compiler label Jul 18, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 18, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 18, 2019
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Jul 18, 2019
@AndrewKushnir
perf(compiler): avoid copying from prototype while cloning an object
4bafcf9 
This commit updates the `_clone` function of the _ApplySourceSpanTransformer class, where the for-in loop was used, causing copying from prototype to own properties and as a result, consuming more memory. Prior to NodeJS 12 (V8 versions before 7.4) there was an optimization that improved the situation and since that logic was removed in favor of other optimizations, the situation with memory consumption causing by the for-in loop got worse. This commit adds a check to make sure we copy only own properties over to cloned object.

Closes angular#31627.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Jul 18, 2019
@AndrewKushnir
perf(compiler): avoid copying from prototype while cloning an object
1c1e55f 
This commit updates the `_clone` function of the `_ApplySourceSpanTransformer` class, where the for-in loop was used, resulting in copying from prototype to own properties, thus consuming more memory. Prior to NodeJS 12 (V8 versions before 7.4) there was an optimization that was improving the situation and since that logic was removed in favor of other optimizations, the situation with memory consumption caused by the for-in loop got worse. This commit adds a check to make sure we copy only own properties over to cloned object.

Closes angular#31627.
@AndrewKushnir AndrewKushnir mentioned this issue Jul 18, 2019
Closed
3 tasks
@AndrewKushnir
Copy link
Contributor

@tebbi thanks a lot for investigation and detailed information provided! 👍

We've created a PR to address this issue (PR #31638).

Is this code only used for Angular builds or does it also affect webpages?

The mentioned code is only used during compilation process and is not invoked on web pages.

Thank you.

@filipesilva
Copy link
Contributor

@AndrewKushnir won't this compiler code also run at browser app runtime when it is using JIT compilation?

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Jul 19, 2019
@AndrewKushnir
perf(compiler): avoid copying from prototype while cloning an object
1b26797 
This commit updates the `_clone` function of the `_ApplySourceSpanTransformer` class, where the for-in loop was used, resulting in copying from prototype to own properties, thus consuming more memory. Prior to NodeJS 12 (V8 versions before 7.4) there was an optimization that was improving the situation and since that logic was removed in favor of other optimizations, the situation with memory consumption caused by the for-in loop got worse. This commit adds a check to make sure we copy only own properties over to cloned object.

Closes angular#31627.
@kara kara closed this as completed in 24ca582 Jul 23, 2019
kara pushed a commit that referenced this issue Jul 23, 2019
@AndrewKushnir @kara
perf(compiler): avoid copying from prototype while cloning an object (#…
1f3daa0 
…31638)

This commit updates the `_clone` function of the `_ApplySourceSpanTransformer` class, where the for-in loop was used, resulting in copying from prototype to own properties, thus consuming more memory. Prior to NodeJS 12 (V8 versions before 7.4) there was an optimization that was improving the situation and since that logic was removed in favor of other optimizations, the situation with memory consumption caused by the for-in loop got worse. This commit adds a check to make sure we copy only own properties over to cloned object.

Closes #31627.

PR Close #31638
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this issue Sep 6, 2019
@AndrewKushnir @sabeersulaiman
perf(compiler): avoid copying from prototype while cloning an object (a…
2ab288d 
…ngular#31638)

This commit updates the `_clone` function of the `_ApplySourceSpanTransformer` class, where the for-in loop was used, resulting in copying from prototype to own properties, thus consuming more memory. Prior to NodeJS 12 (V8 versions before 7.4) there was an optimization that was improving the situation and since that logic was removed in favor of other optimizations, the situation with memory consumption caused by the for-in loop got worse. This commit adds a check to make sure we copy only own properties over to cloned object.

Closes angular#31627.

PR Close angular#31638
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area: compiler Issues related to `ngc`, Angular's template compiler freq3: high type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

perf(compiler): avoid copying from prototype while cloning an object AndrewKushnir/angular
4 participants
@tebbi @filipesilva @alxhub @AndrewKushnir