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

Perf logging & Incremental Compilation #29380

Closed
wants to merge 3 commits into from
Closed

Perf logging & Incremental Compilation #29380

wants to merge 3 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Mar 18, 2019

No description provided.

@alxhub alxhub requested review from a team as code owners March 18, 2019 19:44
petebacondarwin
petebacondarwin reviewed Mar 18, 2019
View reviewed changes
packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts Outdated
@@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ConstantPool} from '@angular/compiler';
import {NOOP_PERF_RECORDER} from '@angular/compiler-cli/src/ngtsc/perf';
Copy link
Member

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 is used.

AndrewKushnir
AndrewKushnir approved these changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great (just left a few nits) 👍
One question/proposal: may be add some test for PerfTracker logic?

packages/compiler-cli/src/ngtsc/perf/src/tracking.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/perf/src/tracking.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/incremental/src/state.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/program.ts Show resolved Hide resolved
packages/compiler-cli/src/transformers/api.ts Show resolved Hide resolved
@mhevery mhevery added the area: core Issues related to the framework runtime label Mar 19, 2019
@ngbot ngbot bot added this to the needsTriage milestone Mar 19, 2019
@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 20, 2019
@alxhub alxhub force-pushed the ngtsc-perf branch 3 times, most recently from b5c8ffe to 9f74b66 Compare March 22, 2019 18:26
@alxhub
Copy link
Member Author

alxhub commented Mar 28, 2019

As far as tests go, I'd rather wait until we stabilize the logging format before we start explicitly testing it.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 28, 2019
AndrewKushnir
AndrewKushnir approved these changes Mar 28, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great (just left a few minor nits)! 👍

packages/compiler-cli/src/ngtsc/incremental/src/state.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/incremental/src/state.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/perf/src/tracking.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/perf/src/tracking.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/program.ts Show resolved Hide resolved
@alxhub
feat(ivy): performance trace mechanism for ngtsc
761aa23 
This commit adds a `tracePerformance` option for tsconfig.json. When
specified, it causes a JSON file with timing information from the ngtsc
compiler to be emitted at the specified path.

This tracing system is used to instrument the analysis/emit phases of
compilation, and will be useful in debugging future integration work with
@angular/cli.

See ngtsc/perf/README.md for more details.
@alxhub
test(ivy): support multiple compilations in the ngtsc test env
9309fba 
This commit adds support for compiling the same program repeatedly in a way
that's similar to how incremental builds work in a tool such as the CLI.

* support is added to the compiler entrypoint for reuse of the Program
  object between compilations. This is the basis of the compiler's
  incremental compilation model.

* support is added to wrap the CompilerHost the compiler creates and cache
  ts.SourceFiles in between compilations.

* support is added to track when files are emitted, for assertion purposes.

* an 'exclude' section is added to the base tsconfig to prevent .d.ts
  outputs from the first compilation from becoming inputs to any subsequent
  compilations.
@alxhub
perf(ivy): basic incremental compilation for ngtsc
3492fc0 
This commit introduces a mechanism for incremental compilation to the ngtsc
compiler.

Previously, incremental information was used in the construction of the
ts.Program for subsequent compilations, but was not used in ngtsc itself.

This commit adds an IncrementalState class, which tracks state between ngtsc
compilations. Currently, this supports skipping the TypeScript emit step
when the compiler can prove the contents of emit have not changed.

This is implemented for @Injectables as well as for files which don't
contain any Angular decorated types. These are the only files which can be
proven to be safe today.

See ngtsc/incremental/README.md for more details.
@AndrewKushnir
Copy link
Contributor

@alxhub thanks for answering questions and for additional changes! Everything looks great, I have no other questions/comments. 👍

@alxhub alxhub added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 1, 2019
@alxhub
Copy link
Member Author

alxhub commented Apr 1, 2019

I am the approver for compiler-cli.

@jasonaden jasonaden closed this in aaa16f2 Apr 1, 2019
jasonaden pushed a commit that referenced this pull request Apr 1, 2019
@alxhub @jasonaden
test(ivy): support multiple compilations in the ngtsc test env (#29380)
7316212 
This commit adds support for compiling the same program repeatedly in a way
that's similar to how incremental builds work in a tool such as the CLI.

* support is added to the compiler entrypoint for reuse of the Program
  object between compilations. This is the basis of the compiler's
  incremental compilation model.

* support is added to wrap the CompilerHost the compiler creates and cache
  ts.SourceFiles in between compilations.

* support is added to track when files are emitted, for assertion purposes.

* an 'exclude' section is added to the base tsconfig to prevent .d.ts
  outputs from the first compilation from becoming inputs to any subsequent
  compilations.

PR Close #29380
jasonaden pushed a commit that referenced this pull request Apr 1, 2019
@alxhub @jasonaden
perf(ivy): basic incremental compilation for ngtsc (#29380)
7041e61 
This commit introduces a mechanism for incremental compilation to the ngtsc
compiler.

Previously, incremental information was used in the construction of the
ts.Program for subsequent compilations, but was not used in ngtsc itself.

This commit adds an IncrementalState class, which tracks state between ngtsc
compilations. Currently, this supports skipping the TypeScript emit step
when the compiler can prove the contents of emit have not changed.

This is implemented for @Injectables as well as for files which don't
contain any Angular decorated types. These are the only files which can be
proven to be safe today.

See ngtsc/incremental/README.md for more details.

PR Close #29380
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@alxhub @wKoza
feat(ivy): performance trace mechanism for ngtsc (angular#29380)
5a7c5cd 
This commit adds a `tracePerformance` option for tsconfig.json. When
specified, it causes a JSON file with timing information from the ngtsc
compiler to be emitted at the specified path.

This tracing system is used to instrument the analysis/emit phases of
compilation, and will be useful in debugging future integration work with
@angular/cli.

See ngtsc/perf/README.md for more details.

PR Close angular#29380
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@alxhub @wKoza
test(ivy): support multiple compilations in the ngtsc test env (angul…
aa55e1a 
…ar#29380)

This commit adds support for compiling the same program repeatedly in a way
that's similar to how incremental builds work in a tool such as the CLI.

* support is added to the compiler entrypoint for reuse of the Program
  object between compilations. This is the basis of the compiler's
  incremental compilation model.

* support is added to wrap the CompilerHost the compiler creates and cache
  ts.SourceFiles in between compilations.

* support is added to track when files are emitted, for assertion purposes.

* an 'exclude' section is added to the base tsconfig to prevent .d.ts
  outputs from the first compilation from becoming inputs to any subsequent
  compilations.

PR Close angular#29380
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@alxhub @wKoza
perf(ivy): basic incremental compilation for ngtsc (angular#29380)
0abe15a 
This commit introduces a mechanism for incremental compilation to the ngtsc
compiler.

Previously, incremental information was used in the construction of the
ts.Program for subsequent compilations, but was not used in ngtsc itself.

This commit adds an IncrementalState class, which tracks state between ngtsc
compilations. Currently, this supports skipping the TypeScript emit step
when the compiler can prove the contents of emit have not changed.

This is implemented for @Injectables as well as for files which don't
contain any Angular decorated types. These are the only files which can be
proven to be safe today.

See ngtsc/incremental/README.md for more details.

PR Close angular#29380
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request May 28, 2019
@filipesilva
feat(@schematics/angular): default Ivy apps to AOT true
beee4f9 
Testing on AIO with Angular master as of 28/05/2019 I got these results:
JIT ~414ms (369, 378, 408, 323, 593)
AOT using VE ~1383ms (1365, 1185, 1767, 1135, 1467)
AOT using Ivy ~517ms (600, 391, 444, 756, 394)

This is largely due to angular/angular#29380 and angular/angular#30238.

The second PR above was not merged to master, and thus will not be in 8.0.0. This PR should be merged to match it.
@filipesilva filipesilva mentioned this pull request May 28, 2019
Merged
filipesilva added a commit to filipesilva/angular that referenced this pull request May 28, 2019
@filipesilva
docs: add default AOT to migrating projects to Ivy
7eb272a 
Followup to angular#29380, angular#30238, angular/angular-cli#14537.
@filipesilva filipesilva mentioned this pull request May 28, 2019
Closed
14 tasks
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request May 28, 2019
@filipesilva
feat(@schematics/angular): default Ivy apps to AOT true
410070a 
Testing on AIO with Angular master as of 28/05/2019 I got these results:
JIT ~414ms (369, 378, 408, 323, 593)
AOT using VE ~1383ms (1365, 1185, 1767, 1135, 1467)
AOT using Ivy ~517ms (600, 391, 444, 756, 394)

This is largely due to angular/angular#29380 and angular/angular#30238.

The second PR above was not merged to master, and thus will not be in 8.0.0. This PR should be merged to match it.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request May 28, 2019
@filipesilva
feat(@schematics/angular): default Ivy apps to AOT true
86e69d5 
Testing on AIO with Angular master as of 28/05/2019 I got these results:
JIT ~414ms (369, 378, 408, 323, 593)
AOT using VE ~1383ms (1365, 1185, 1767, 1135, 1467)
AOT using Ivy ~517ms (600, 391, 444, 756, 394)

This is largely due to angular/angular#29380 and angular/angular#30238.

The second PR above was not merged to master, and thus will not be in 8.0.0. This PR should be merged to match it.
IgorMinar pushed a commit that referenced this pull request May 28, 2019
@filipesilva @IgorMinar
docs: add default AOT to migrating projects to Ivy (#30697)
875eedb 
Followup to #29380, #30238, angular/angular-cli#14537.

PR Close #30697
IgorMinar pushed a commit that referenced this pull request May 28, 2019
@filipesilva @IgorMinar
docs: add default AOT to migrating projects to Ivy (#30697)
002fbdd 
Followup to #29380, #30238, angular/angular-cli#14537.

PR Close #30697
alexeagle pushed a commit to angular/angular-cli that referenced this pull request Jun 6, 2019
@filipesilva @alexeagle
feat(@schematics/angular): default Ivy apps to AOT true
24f37a6 
Testing on AIO with Angular master as of 28/05/2019 I got these results:
JIT ~414ms (369, 378, 408, 323, 593)
AOT using VE ~1383ms (1365, 1185, 1767, 1135, 1467)
AOT using Ivy ~517ms (600, 391, 444, 756, 394)

This is largely due to angular/angular#29380 and angular/angular#30238.

The second PR above was not merged to master, and thus will not be in 8.0.0. This PR should be merged to match it.
alexeagle pushed a commit to angular/angular-cli that referenced this pull request Jun 6, 2019
@filipesilva @alexeagle
feat(@schematics/angular): default Ivy apps to AOT true
9f45745 
Testing on AIO with Angular master as of 28/05/2019 I got these results:
JIT ~414ms (369, 378, 408, 323, 593)
AOT using VE ~1383ms (1365, 1185, 1767, 1135, 1467)
AOT using Ivy ~517ms (600, 391, 444, 756, 394)

This is largely due to angular/angular#29380 and angular/angular#30238.

The second PR above was not merged to master, and thus will not be in 8.0.0. This PR should be merged to match it.
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alxhub @AndrewKushnir @petebacondarwin @mhevery @googlebot