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

Remove side effects to improve Ivy tree shaking in Closure #35769

Closed
wants to merge 5 commits into from

Conversation

@dgp1130
Copy link
Contributor

dgp1130 commented Feb 29, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Closure does not tree shake unused components built with Ivy.

Issue Number: http://b/149874196, http://b/149874638.

What is the new behavior?

Closure will now tree shake unused components built with Ivy.

Does this PR introduce a breaking change?

  • No

Other information

Closure is failing to tree shake components because it is detecting side effects in the definition which it is unable to remove. Most of these are fixed by adding noSideEffects() to the relevant calls and generally tweaking things so Closure has a better understanding of what can be stripped if not used.

I made separate commits so that each solves its own side effect problem. Let me know if I should squash any/all of these commits together. Also the NgOnChangesFeature touches the compiler as well as Core, so I'm not sure what to put in the commit message. Let me know if I should change that somehow.

…inner function

This is useful for propagating return values without them being converted to a string. It still provides the same guarantees to Closure, which will assume that the function invoked is pure and can be tree-shaken accordingly.
@dgp1130 dgp1130 requested review from mhevery and alxhub Feb 29, 2020
@ngbot ngbot bot modified the milestone: needsTriage Feb 29, 2020
@dgp1130 dgp1130 changed the title Side effects Remove side effects to improve Ivy tree shaking in Closure Feb 29, 2020
@googlebot googlebot added the cla: yes label Feb 29, 2020
Copy link
Member

IgorMinar left a comment

this looks great @dgp1130 !! it needs just a bit more polish but structurally all these changes look solid!

also thanks for documenting the reasoning behind each change and breaking them up into individual commits.

some small things to fix:

FAIL: Commit 11a6a58 uncompressed bundle fell below expected size by 500 bytes or >1% (expected: 175498, actual: 170618).
If this is a desired change, please update the size limits in file '/home/circleci/ng/integration/../integration/_payload-limits.json'.
  • please update integration/_payload-limits.json with the new decreased value (170618) for the closure limit.
  • please run yarn gulp format to reformat the code changes
  • please run yarn bazel run --config=ivy //packages/core/test/bundling/cyclic_import:symbol_test.accept to update golden files
  • please run g3 presubmit once github CI is green

GLOBAL APPROVAL unless further significant modifications are made.

packages/core/src/util/closure.ts Show resolved Hide resolved
dgp1130 added 2 commits Feb 21, 2020
This causes all the `make*Decorator()` functions to be considered pure and to be eligible for associated tree shaking by Closure.
This marks the function are "pure" and eligible to be tree shaken by Closure. Without this, initializing `ngDevMode` is considered a side effect which prevents this function from being tree shaken and also any component which calls it.
@dgp1130 dgp1130 force-pushed the dgp1130:side-effects branch 3 times, most recently from 4aa972e to bfa0120 Mar 2, 2020
dgp1130 added 2 commits Feb 21, 2020
`ɵɵNgOnChangesFeature()` would set `ngInherit`, which is a side effect and also not necessary. This was pulled out to module scope so the function itself can be pure. Since it only curries another function, the call is entirely unnecessary. Updated the compiler to only generate a reference to this function, rather than a call to it, and removed the extra curry indirection.
`ɵɵgetInheritedFactory()` is called from generated code for a component which extends another class. This function is detected by Closure to have a side effect and is not able to tree shake the component as a result. Marking it with `noSideEffects()` tells Closure it can remove this function under the relevant tree shaking conditions.
@dgp1130 dgp1130 force-pushed the dgp1130:side-effects branch from bfa0120 to b2319d9 Mar 2, 2020
@dgp1130 dgp1130 removed request for mhevery and alxhub Mar 3, 2020
@dgp1130

This comment has been minimized.

Copy link
Contributor Author

dgp1130 commented Mar 3, 2020

Addressed your comments @IgorMinar, thanks for the direction on updating goldens. Applied the golden updates to the latest commit. I had to force g3 to pass as the only failures were pre-existing and flaky.

Should be ready to merge.

@atscott atscott closed this in 4052dd8 Mar 3, 2020
atscott added a commit that referenced this pull request Mar 3, 2020
)

This causes all the `make*Decorator()` functions to be considered pure and to be eligible for associated tree shaking by Closure.

PR Close #35769
atscott added a commit that referenced this pull request Mar 3, 2020
This marks the function are "pure" and eligible to be tree shaken by Closure. Without this, initializing `ngDevMode` is considered a side effect which prevents this function from being tree shaken and also any component which calls it.

PR Close #35769
atscott added a commit that referenced this pull request Mar 3, 2020
`ɵɵNgOnChangesFeature()` would set `ngInherit`, which is a side effect and also not necessary. This was pulled out to module scope so the function itself can be pure. Since it only curries another function, the call is entirely unnecessary. Updated the compiler to only generate a reference to this function, rather than a call to it, and removed the extra curry indirection.

PR Close #35769
atscott added a commit that referenced this pull request Mar 3, 2020
`ɵɵgetInheritedFactory()` is called from generated code for a component which extends another class. This function is detected by Closure to have a side effect and is not able to tree shake the component as a result. Marking it with `noSideEffects()` tells Closure it can remove this function under the relevant tree shaking conditions.

PR Close #35769
dgp1130 added a commit to dgp1130/angular that referenced this pull request Mar 4, 2020
…inner function (angular#35769)

This is useful for propagating return values without them being converted to a string. It still provides the same guarantees to Closure, which will assume that the function invoked is pure and can be tree-shaken accordingly.

PR Close angular#35769

(cherry picked from commit 4052dd8)
dgp1130 added a commit to dgp1130/angular that referenced this pull request Mar 4, 2020
…ular#35769)

This causes all the `make*Decorator()` functions to be considered pure and to be eligible for associated tree shaking by Closure.

PR Close angular#35769

(cherry picked from commit dc6a791)
dgp1130 added a commit to dgp1130/angular that referenced this pull request Mar 4, 2020
)

This marks the function are "pure" and eligible to be tree shaken by Closure. Without this, initializing `ngDevMode` is considered a side effect which prevents this function from being tree shaken and also any component which calls it.

PR Close angular#35769

(cherry picked from commit ba36127)
dgp1130 added a commit to dgp1130/angular that referenced this pull request Mar 4, 2020
…#35769)

`ɵɵNgOnChangesFeature()` would set `ngInherit`, which is a side effect and also not necessary. This was pulled out to module scope so the function itself can be pure. Since it only curries another function, the call is entirely unnecessary. Updated the compiler to only generate a reference to this function, rather than a call to it, and removed the extra curry indirection.

PR Close angular#35769

(cherry picked from commit 9cf85d2)
dgp1130 added a commit to dgp1130/angular that referenced this pull request Mar 4, 2020
…r#35769)

`ɵɵgetInheritedFactory()` is called from generated code for a component which extends another class. This function is detected by Closure to have a side effect and is not able to tree shake the component as a result. Marking it with `noSideEffects()` tells Closure it can remove this function under the relevant tree shaking conditions.

PR Close angular#35769

(cherry picked from commit c195d22)
matsko added a commit that referenced this pull request Mar 6, 2020
…inner function (#35769) (#35846)

This is useful for propagating return values without them being converted to a string. It still provides the same guarantees to Closure, which will assume that the function invoked is pure and can be tree-shaken accordingly.

PR Close #35769

(cherry picked from commit 4052dd8)

PR Close #35846
matsko added a commit that referenced this pull request Mar 6, 2020
) (#35846)

This causes all the `make*Decorator()` functions to be considered pure and to be eligible for associated tree shaking by Closure.

PR Close #35769

(cherry picked from commit dc6a791)

PR Close #35846
matsko added a commit that referenced this pull request Mar 6, 2020
…35846)

This marks the function are "pure" and eligible to be tree shaken by Closure. Without this, initializing `ngDevMode` is considered a side effect which prevents this function from being tree shaken and also any component which calls it.

PR Close #35769

(cherry picked from commit ba36127)

PR Close #35846
matsko added a commit that referenced this pull request Mar 6, 2020
…#35846)

`ɵɵNgOnChangesFeature()` would set `ngInherit`, which is a side effect and also not necessary. This was pulled out to module scope so the function itself can be pure. Since it only curries another function, the call is entirely unnecessary. Updated the compiler to only generate a reference to this function, rather than a call to it, and removed the extra curry indirection.

PR Close #35769

(cherry picked from commit 9cf85d2)

PR Close #35846
matsko added a commit that referenced this pull request Mar 6, 2020
#35846)

`ɵɵgetInheritedFactory()` is called from generated code for a component which extends another class. This function is detected by Closure to have a side effect and is not able to tree shake the component as a result. Marking it with `noSideEffects()` tells Closure it can remove this function under the relevant tree shaking conditions.

PR Close #35769

(cherry picked from commit c195d22)

PR Close #35846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.