Compiler "neverEmit" mode for faster turnarounds on external template updates#39679
Compiler "neverEmit" mode for faster turnarounds on external template updates#39679ayazhafiz wants to merge 5 commits into
Conversation
A change to an external template should not cause a recompilation of the
transient dependencies of that template, as the template as "local" to
the components it belongs to. This is a desirable feature of a "fast
path" for the language service, which requires no additional work today
because incremental builds only consider the resource dependencies
attributed to a source file, and imports to that source file, when
deciding whether a source file has been changed. That is, since the
resource file attributed to an external template is not registered as a
dependency of a transient component, such components are not marked
stale on template change. However, the current test suite does not
demonstrate this behavior; this commit adds a test to do so.
Future work includes the following:
- Template updates should not require re-analysis of NgModules when
the component a template belongs to is not to be emitted (i.e. in a
context like a language service, which is readonly)
- Provide to the incremental driver granular information about what
has been changed in a source file. In this way, we can propogate
this optimization to inline templates, for whose source files we
currently entirely re-analyze (and do so for all its dependents).
Today, the Angular compiler performs analysis and resolution in one pass
of the compilation, and uses the same dependency graph for both analysis
and resolution. However, while contexts like a language service need to
"stay in line" with all the analysis work done for a "regular"
compilation, they do not necessarily need all the same resolution work
done, as much of this work is set-up required for emit. Emit is not
relevant in contexts like the language service, which only use the front
and middle of the compiler.
We would like to avoid doing such work where it is not necessary, and we
cannot also disable resolution in such contexts entirely, as resolution
is responsible for bringing in diagnostics we may care about. To this
end, this commit introduces a `neverEmit` flag on `NgCompilerAdapter`s
provided to a compiler instance. `neverEmit` is stricter than
`ignoreForEmit`, as it tells the compiler that the consumer will never
try to emit any compilation results. As the compiler can use this for
elision of work only needed for emit, running an incremental build with
a change in `neverEmit` to `false` will quickly break program
correctness.
Future work:
- Use knowledge of never-emission to avoid doing work to detect cycles
in analysis of traits.
|
@JoostK you will probably be interested in this |
|
Few observations:
Additionally, the cycle detection may start reporting diagnostics in the future depending on how we decide to deal with them. It may mean that this optimization may become invalid at that point. |
I had some thought similar to this, but it brings other questions as well. For example if we have to modify the compiler options, what happens if someone passes
With regard to this - I think I will just drop the changes introduced by the third commit. Since it is unclear what will happen later on, we should probably have some discussion beforehand. Also, I added the cycle check stuff because you had previously mentioned that was a big time suck, but now I am thinking I want to do some benchmarks here before moving forward. Even without this I think we have in a win and removing NgModules from dependency consideration, and my guess is that parsing and AST walks elsewhere will take up 90% of the time, and maybe what we can do here is a 10% win for the common case. Edit: actually for the third commit I will keep the tests, since that seems good to have anyway and went through the trouble of making them :) |
Remote scoping is an alternative to producing cycles during the emit of an Angular program. Whether a component should use remote scoping is determined during resolution, but there are no unit tests verifying this behavior. This commit adds one. Use case is that later on we may remove cycle detection in cases where we don't need it (i.e. there is no emission).
411497a to
9ca029d
Compare
|
|
||
| const deps = compiler.getResourceDependencies(getSourceFileOrError(program, MODULE)); | ||
| expect(deps.length).toBe(0); | ||
| expect(deps).toEqual(jasmine.arrayContaining([])); |
There was a problem hiding this comment.
nit: this assertion is a little redundant :)
There was a problem hiding this comment.
ahaha i don’t even remember writing this! thanks.
| new NoopIncrementalBuildStrategy(), /** enableTemplateTypeChecker */ false); | ||
|
|
||
| const deps = compiler.getResourceDependencies(getSourceFileOrError(program, MODULE)); | ||
| expect(deps.length).toBe(0); |
There was a problem hiding this comment.
Dependency analysis would be a common situation that would never emit any code but this change appears to prevent full dependency analysis from working. I think a different name may be more intuitive as to its behavior or, alternatively, the ability to perform full dependency analysis should be retained.
There was a problem hiding this comment.
I am conflicted on this. Retaining the ability to perform full dependency analysis eliminates the optimization, and then all of this is a non-starter. On the other hand, providing a more intuitive name for this option suggests that later optimizations we may make for this kind of situation (avoiding unnecessary work we generally do for templates when the template will never be emitted) should also be transparent to the caller, which would make the optimizations "tunable" by the caller, which falls apart correctness-wise very quickly.
I think we should keep this as a "black box" optimization flag in the compiler, saying "okay, we won't do work you don't need for emit, but we also don't promise that you have enough information you need for emit", the condition of "emit" being very important here. I will try to document this contract better, but for example we know that the optimization performed here is relevant only for the "remote scoping" feature that takes place during emit. In other contexts I would not expect an NgModule to depend on a template file.
Maybe a more intuitive way to break this down would be to separate the analysis/resolution (resolution being information necessary for emit) dependencies into being handled by two different pipelines, but this requires significant rework and is probably not worth it IMO.
There was a problem hiding this comment.
Based on that, I think that the option probably is not "never emit" since the option breaks legitimate use cases that would never emit (and could potentially break more in the future?). I think this option is more of a special "language service mode" that is intended to optimize specific behavior needed by the language service and makes no guarantees about any functionality not needed by the language service. So maybe the option should be named languageServiceMode? This has the advantage of very little ambiguity as to its function/purpose whereas neverEmit could very easily be used in a context that would cause unintended/unexpected behavior.
There was a problem hiding this comment.
Yeah, that sounds like a good idea to me.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
See individual commits for details, and https://docs.google.com/document/d/1l1UUMC4vzrIqY1-S_jJ-LT-iJJWEVZcDhcUP8yJlubg/edit?usp=sharing.
Part one of streamlining incremental builds + diagnostics for the language service, and sunsetting
overrideTemplateon the template typechecker.