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

fix(ivy): always re-analyze the program during incremental rebuilds #33862

Closed
wants to merge 3 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Nov 15, 2019

Previously, the ngtsc compiler attempted to reuse analysis work from the
previous program during an incremental build. To do this, it had to prove
that the work was safe to reuse - that no changes made to the new program
would invalidate the previous analysis.

The implementation of this had a significant design flaw: if the previous
program had errors, the previous analysis would be missing significant
information, and the dependency graph extracted from it would not be
sufficient to determine which files should be re-analyzed to fill in the
gaps. This often meant that the build output after an error was resolved
would be wholly incorrect.

This commit switches ngtsc to take a simpler approach to incremental
rebuilds. Instead of attempting to reuse prior analysis work, the entire
program is re-analyzed with each compilation. This is actually not as
expensive as one might imagine - analysis is a fairly small part of overall
compilation time.

Based on the dependency graph extracted during this analysis, the compiler
then can make accurate decisions on whether to emit specific files. A new
suite of tests is added to validate behavior in the presence of source code
level errors.

This new approach is dramatically simpler than the previous algorithm, and
should always produce correct results for a semantically correct program.

@googlebot googlebot added the cla: yes label Nov 15, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/inc/errors branch from 5458b38 to ef19155 Nov 15, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/inc/errors branch from ef19155 to 8ec502e Nov 15, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/inc/errors branch 3 times, most recently from d0606e3 to ff1066d Nov 16, 2019
@alxhub alxhub marked this pull request as ready for review Nov 18, 2019
@alxhub alxhub requested a review from angular/fw-compiler as a code owner Nov 18, 2019
@JoostK
JoostK approved these changes Nov 19, 2019
Copy link
Member

JoostK left a comment

LGTM once comments have been addressed.

@@ -56,14 +56,33 @@ export function main(
}

export function mainDiagnosticsForTest(
args: string[], config?: NgcParsedConfiguration): ReadonlyArray<ts.Diagnostic|api.Diagnostic> {
args: string[], config?: NgcParsedConfiguration,
programReuse?: {program: api.Program | undefined},

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 19, 2019

Member

Why is this wrapped into an object literal? It seems this can just be:

Suggested change
programReuse?: {program: api.Program | undefined},
programReuse?: api.Program,

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 19, 2019

Author Contributor

It's an input/output parameter. We're returning the new program back to the caller via the reference in this object. Not the best API; we'll overhaul it once VE no longer goes through it too.

packages/compiler-cli/src/main.ts Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/program.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/program.ts Outdated Show resolved Hide resolved
alxhub added 3 commits Nov 12, 2019
Previously, the ngtsc compiler attempted to reuse analysis work from the
previous program during an incremental build. To do this, it had to prove
that the work was safe to reuse - that no changes made to the new program
would invalidate the previous analysis.

The implementation of this had a significant design flaw: if the previous
program had errors, the previous analysis would be missing significant
information, and the dependency graph extracted from it would not be
sufficient to determine which files should be re-analyzed to fill in the
gaps. This often meant that the build output after an error was resolved
would be wholly incorrect.

This commit switches ngtsc to take a simpler approach to incremental
rebuilds. Instead of attempting to reuse prior analysis work, the entire
program is re-analyzed with each compilation. This is actually not as
expensive as one might imagine - analysis is a fairly small part of overall
compilation time.

Based on the dependency graph extracted during this analysis, the compiler
then can make accurate decisions on whether to emit specific files. A new
suite of tests is added to validate behavior in the presence of source code
level errors.

This new approach is dramatically simpler than the previous algorithm, and
should always produce correct results for a semantically correct program.s

Fixes #32388
Fixes #32214
This commit changes the reporting of watch mode diagnostics for ngtsc to use
the same formatting as non-watch mode diagnostics. This prints rich and
contextual errors even in watch mode, which previously was not the case.

Fixes #32213
@alxhub alxhub force-pushed the alxhub:ngtsc/inc/errors branch from ff1066d to ce4d4a6 Nov 19, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Nov 20, 2019

alxhub added a commit that referenced this pull request Nov 20, 2019
alxhub added a commit that referenced this pull request Nov 20, 2019
…33862)

Previously, the ngtsc compiler attempted to reuse analysis work from the
previous program during an incremental build. To do this, it had to prove
that the work was safe to reuse - that no changes made to the new program
would invalidate the previous analysis.

The implementation of this had a significant design flaw: if the previous
program had errors, the previous analysis would be missing significant
information, and the dependency graph extracted from it would not be
sufficient to determine which files should be re-analyzed to fill in the
gaps. This often meant that the build output after an error was resolved
would be wholly incorrect.

This commit switches ngtsc to take a simpler approach to incremental
rebuilds. Instead of attempting to reuse prior analysis work, the entire
program is re-analyzed with each compilation. This is actually not as
expensive as one might imagine - analysis is a fairly small part of overall
compilation time.

Based on the dependency graph extracted during this analysis, the compiler
then can make accurate decisions on whether to emit specific files. A new
suite of tests is added to validate behavior in the presence of source code
level errors.

This new approach is dramatically simpler than the previous algorithm, and
should always produce correct results for a semantically correct program.s

Fixes #32388
Fixes #32214

PR Close #33862
alxhub added a commit that referenced this pull request Nov 20, 2019
This commit changes the reporting of watch mode diagnostics for ngtsc to use
the same formatting as non-watch mode diagnostics. This prints rich and
contextual errors even in watch mode, which previously was not the case.

Fixes #32213

PR Close #33862
@alxhub alxhub closed this in cf9aa4f Nov 20, 2019
alxhub added a commit that referenced this pull request Nov 20, 2019
…33862)

Previously, the ngtsc compiler attempted to reuse analysis work from the
previous program during an incremental build. To do this, it had to prove
that the work was safe to reuse - that no changes made to the new program
would invalidate the previous analysis.

The implementation of this had a significant design flaw: if the previous
program had errors, the previous analysis would be missing significant
information, and the dependency graph extracted from it would not be
sufficient to determine which files should be re-analyzed to fill in the
gaps. This often meant that the build output after an error was resolved
would be wholly incorrect.

This commit switches ngtsc to take a simpler approach to incremental
rebuilds. Instead of attempting to reuse prior analysis work, the entire
program is re-analyzed with each compilation. This is actually not as
expensive as one might imagine - analysis is a fairly small part of overall
compilation time.

Based on the dependency graph extracted during this analysis, the compiler
then can make accurate decisions on whether to emit specific files. A new
suite of tests is added to validate behavior in the presence of source code
level errors.

This new approach is dramatically simpler than the previous algorithm, and
should always produce correct results for a semantically correct program.s

Fixes #32388
Fixes #32214

PR Close #33862
alxhub added a commit that referenced this pull request Nov 20, 2019
This commit changes the reporting of watch mode diagnostics for ngtsc to use
the same formatting as non-watch mode diagnostics. This prints rich and
contextual errors even in watch mode, which previously was not the case.

Fixes #32213

PR Close #33862
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.