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

perf(ivy): eagerly parse the template twice during analysis #34334

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 10, 2019

A quirk of the Angular template parser is that when parsing templates in the
"default" mode, with options specified by the user, the source mapping
information in the template AST may be inaccurate. As a result, the compiler
parses the template twice: once for "emit" and once to produce an AST with
accurate sourcemaps for diagnostic production.

Previously, only the first parse was performed during analysis. The second
parse occurred during the template type-checking phase, just in time to
produce the template type-checking file.

However, with the reuse of analysis results during incremental builds, it
makes more sense to do the diagnostic parse eagerly during analysis so that
the work isn't unnecessarily repeated in subsequent builds. This commit
refactors the ComponentDecoratorHandler to do both parses eagerly, which
actually cleans up some complexity around template parsing as well.

@googlebot googlebot added the cla: yes label Dec 10, 2019
@JoostK
JoostK approved these changes Dec 10, 2019
Copy link
Member

JoostK left a comment

Much better!

@@ -737,14 +696,41 @@ export class ComponentDecoratorHandler implements
interpolation = InterpolationConfig.fromArray(value as[string, string]);
}

const {errors, nodes: emitNodes, styleUrls, styles} = parseTemplate(templateStr, templateUrl, {

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 10, 2019

Member

It would make more sense to extract errors from the secondary parse.

preserveWhitespaces: true,
interpolationConfig: interpolation,
range: templateRange, escapedString,
enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat,

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 10, 2019

Member

I think we should be able to set this to false, always. It avoids computing message digests that we're not going to use, anyway.

@ngbot ngbot bot added this to the needsTriage milestone Dec 11, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/template-parse-up-front branch from a820420 to 3452bc2 Dec 12, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 12, 2019

A quirk of the Angular template parser is that when parsing templates in the
"default" mode, with options specified by the user, the source mapping
information in the template AST may be inaccurate. As a result, the compiler
parses the template twice: once for "emit" and once to produce an AST with
accurate sourcemaps for diagnostic production.

Previously, only the first parse was performed during analysis. The second
parse occurred during the template type-checking phase, just in time to
produce the template type-checking file.

However, with the reuse of analysis results during incremental builds, it
makes more sense to do the diagnostic parse eagerly during analysis so that
the work isn't unnecessarily repeated in subsequent builds. This commit
refactors the `ComponentDecoratorHandler` to do both parses eagerly, which
actually cleans up some complexity around template parsing as well.
@alxhub alxhub force-pushed the alxhub:ngtsc/template-parse-up-front branch from 3452bc2 to a050bcf Dec 12, 2019
@alxhub alxhub marked this pull request as ready for review Dec 12, 2019
@alxhub alxhub requested review from angular/fw-compiler as code owners Dec 12, 2019
kara added a commit that referenced this pull request Dec 12, 2019
A quirk of the Angular template parser is that when parsing templates in the
"default" mode, with options specified by the user, the source mapping
information in the template AST may be inaccurate. As a result, the compiler
parses the template twice: once for "emit" and once to produce an AST with
accurate sourcemaps for diagnostic production.

Previously, only the first parse was performed during analysis. The second
parse occurred during the template type-checking phase, just in time to
produce the template type-checking file.

However, with the reuse of analysis results during incremental builds, it
makes more sense to do the diagnostic parse eagerly during analysis so that
the work isn't unnecessarily repeated in subsequent builds. This commit
refactors the `ComponentDecoratorHandler` to do both parses eagerly, which
actually cleans up some complexity around template parsing as well.

PR Close #34334
@kara kara closed this in af95ddd Dec 12, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 12, 2020

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 Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.