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

Jump to bottom

perf: AOT compilation optimizations #38539

Closed
wants to merge 2 commits into from
Closed

perf: AOT compilation optimizations #38539

wants to merge 2 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Aug 19, 2020

See individual commits

@JoostK JoostK added area: performance refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Aug 19, 2020
@ngbot ngbot bot added this to the needsTriage milestone Aug 19, 2020
@JoostK JoostK marked this pull request as ready for review August 19, 2020 21:18
@JoostK JoostK requested a review from alxhub August 19, 2020 21:18
alxhub
alxhub approved these changes Aug 31, 2020
View reviewed changes
packages/compiler-cli/src/ngtsc/annotations/src/component.ts
// BoundTarget, which is similar to a ts.TypeChecker.
const binder = new R3TargetBinder(matcher);
const bound = binder.bind({template: metadata.template.nodes});

// The BoundTarget knows which directives and pipes matched the template.
const usedDirectives = bound.getUsedDirectives();
const usedPipes = bound.getUsedPipes().map(name => pipes.get(name)!);
const usedDirectives = bound.getUsedDirectives().map(directive => {
Copy link
Member

Choose a reason for hiding this comment

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

What if instead, we make expression inside the directive metadata into a closure which produces the expression only on demand? That would fix the performance issues and simplify all the downstream logic, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually quite like the way it currently is. The result of the closure would ideally be memoized to avoid repeated computations. This is certainly something we could do; I just think I prefer this straightforward post-processing of the matched directives. Pipes need to be processed in the same way anyway, so currently it's nicely symmetrical. Additionally, the SelectorMatcher is currently context-free so could theoretically also be cached (but the benefits of that are quite small).

If you feel like we should continue capturing all directives in the resolve metadata (for subsequent compilation step to use) then it has to become a closure. Currently, storing only the reduced set is actually beneficial to perf downstream as there will be less directives to match in TemplateDefinitionBuilder (actually that becomes a noop as all directives will be matched) and the linker work also benefits from not having to do directive matching itself.

@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Aug 31, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Sep 3, 2020
@JoostK JoostK force-pushed the perf/directive-refs branch 3 times, most recently from 8d4feef to 31c3d62 Compare September 8, 2020 21:03
@JoostK JoostK 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 Sep 8, 2020
@JoostK
Copy link
Member Author

JoostK commented Sep 8, 2020

merge-assistance: presubmit was green; this was rebased to resolve a conflict in an import declaration.

@atscott atscott added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 8, 2020
@atscott atscott closed this in 4faac78 Sep 8, 2020
atscott pushed a commit that referenced this pull request Sep 8, 2020
@JoostK @atscott
perf(compiler-cli): optimize computation of type-check scope informat…
ba95b79 
…ion (#38539)

When type-checking a component, the declaring NgModule scope is used
to create a directive matcher that contains flattened directive metadata,
i.e. the metadata of a directive and its base classes. This computation
is done for all components, whereas the type-check scope is constant per
NgModule. Additionally, the flattening of metadata is constant per
directive instance so doesn't necessarily have to be recomputed for
each component.

This commit introduces a `TypeCheckScopes` class that is responsible
for flattening directives and computing the scope per NgModule. It
caches the computed results as appropriate to avoid repeated computation.

PR Close #38539
@JoostK JoostK mentioned this pull request Sep 8, 2020
Closed
atscott added a commit to atscott/angular that referenced this pull request Sep 9, 2020
@atscott
Revert "perf(compiler-cli): optimize computation of type-check scope …
a39f211 
…information (angular#38539)"

This reverts commit ba95b79.
internal failure:
https://test.corp.google.com/ui#id=OCL:329948619:BASE:329967516:1599160428139:d63165ae
atscott added a commit to atscott/angular that referenced this pull request Sep 9, 2020
@atscott
Revert "perf(compiler-cli): only emit directive/pipe references that …
cb1d097 
…are used (angular#38539)"

This reverts commit 4faac78.
internal failure:
https://test.corp.google.com/ui#id=OCL:329948619:BASE:329967516:1599160428139:d63165ae
@atscott atscott mentioned this pull request Sep 9, 2020
Closed
atscott added a commit that referenced this pull request Sep 9, 2020
@atscott
Revert "perf(compiler-cli): only emit directive/pipe references that …
18f84a0 
…are used (#38539)" (#38765)

This reverts commit 4faac78.
internal failure:
https://test.corp.google.com/ui#id=OCL:329948619:BASE:329967516:1599160428139:d63165ae

PR Close #38765
@JoostK JoostK reopened this Sep 9, 2020
@JoostK JoostK added state: WIP and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Sep 9, 2020
JoostK added a commit to JoostK/angular that referenced this pull request Sep 11, 2020
@JoostK
fix(compiler): detect pipes in ICUs in template binder
30151c3 
Recent work on compiler internals in angular#38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.

The issue occurred because the change in angular#38539 would determine the set
of used pipes up-front, independent from the template compilation using
the `R3TargetBinder`. However, `R3TargetBinder` did not consider
expressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks angular#38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only `R3TargetBinder`.
@JoostK JoostK mentioned this pull request Sep 11, 2020
Closed
JoostK added a commit to JoostK/angular that referenced this pull request Sep 11, 2020
@JoostK
fix(compiler): detect pipes in ICUs in template binder
99af4c6 
Recent work on compiler internals in angular#38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.

The issue occurred because the change in angular#38539 would determine the set
of used pipes up-front, independent from the template compilation using
the `R3TargetBinder`. However, `R3TargetBinder` did not consider
expressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks angular#38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only `R3TargetBinder`.
JoostK added a commit to JoostK/angular that referenced this pull request Sep 11, 2020
@JoostK
fix(compiler): detect pipes in ICUs in template binder
0fd8ae3 
Recent work on compiler internals in angular#38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.

The issue occurred because the change in angular#38539 would determine the set
of used pipes up-front, independent from the template compilation using
the `R3TargetBinder`. However, `R3TargetBinder` did not consider
expressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks angular#38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only `R3TargetBinder`.
@JoostK
Copy link
Member Author

JoostK commented Sep 11, 2020

Blocked on #38810. Once that is in, it's probably a good idea to run a global presubmit for this one so that there's less risk of needing to revert again.

AndrewKushnir pushed a commit that referenced this pull request Sep 11, 2020
@JoostK @AndrewKushnir
fix(compiler): detect pipes in ICUs in template binder (#38810)
ec2dbe7 
Recent work on compiler internals in #38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.

The issue occurred because the change in #38539 would determine the set
of used pipes up-front, independent from the template compilation using
the `R3TargetBinder`. However, `R3TargetBinder` did not consider
expressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks #38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only `R3TargetBinder`.

PR Close #38810
AndrewKushnir pushed a commit that referenced this pull request Sep 11, 2020
@JoostK @AndrewKushnir
fix(compiler): detect pipes in ICUs in template binder (#38810)
15207e3 
Recent work on compiler internals in #38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.

The issue occurred because the change in #38539 would determine the set
of used pipes up-front, independent from the template compilation using
the `R3TargetBinder`. However, `R3TargetBinder` did not consider
expressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks #38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only `R3TargetBinder`.

PR Close #38810
@AndrewKushnir
Copy link
Contributor

@JoostK FYI #38810 is now merged. Could you please rebase this PR? I'll start global presubmit once PR is rebased. Thank you.

@JoostK
perf(compiler-cli): only emit directive/pipe references that are used
4c18011 
For the compilation of a component, the compiler has to prepare some
information about the directives and pipes that are used in the template.
This information includes an expression for directives/pipes, for usage
within the compilation output. For large NgModule compilation scopes
this has shown to introduce a performance hotspot, as the generation of
expressions is quite expensive. This commit reduces the performance
overhead by only generating expressions for the directives/pipes that
are actually used within the template, significantly cutting down on
the compiler's resolve phase.
@JoostK
perf(compiler-cli): optimize computation of type-check scope information
faeeea1 
When type-checking a component, the declaring NgModule scope is used
to create a directive matcher that contains flattened directive metadata,
i.e. the metadata of a directive and its base classes. This computation
is done for all components, whereas the type-check scope is constant per
NgModule. Additionally, the flattening of metadata is constant per
directive instance so doesn't necessarily have to be recomputed for
each component.

This commit introduces a `TypeCheckScopes` class that is responsible
for flattening directives and computing the scope per NgModule. It
caches the computed results as appropriate to avoid repeated computation.
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 14, 2020
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Sep 14, 2020 •
edited

Presubmit + Global Presubmit (with Ivy).

@AndrewKushnir
Copy link
Contributor

FYI, presubmits (including a global one) went well.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 14, 2020
AndrewKushnir pushed a commit that referenced this pull request Sep 14, 2020
@JoostK @AndrewKushnir
perf(compiler-cli): optimize computation of type-check scope informat…
297c060 
…ion (#38539)

When type-checking a component, the declaring NgModule scope is used
to create a directive matcher that contains flattened directive metadata,
i.e. the metadata of a directive and its base classes. This computation
is done for all components, whereas the type-check scope is constant per
NgModule. Additionally, the flattening of metadata is constant per
directive instance so doesn't necessarily have to be recomputed for
each component.

This commit introduces a `TypeCheckScopes` class that is responsible
for flattening directives and computing the scope per NgModule. It
caches the computed results as appropriate to avoid repeated computation.

PR Close #38539
@JoostK JoostK mentioned this pull request Sep 14, 2020
Closed
@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 Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: performance cla: yes refactoring Issue that involves refactoring or code-cleanup 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
@JoostK @AndrewKushnir @alxhub @atscott @googlebot