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

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

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

Comments

@tebbi
Copy link

commented Jul 18, 2019

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?

@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

perf(compiler): avoid copying from prototype while cloning an object
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

perf(compiler): avoid copying from prototype while cloning an object
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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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

perf(compiler): avoid copying from prototype while cloning an object
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 in 24ca582 Jul 23, 2019

kara added a commit that referenced this issue Jul 23, 2019

perf(compiler): avoid copying from prototype while cloning an object (#…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.