Performance optimizations #1764
Conversation
src/lib/ts/tsconfig.ts
Outdated
| @@ -32,6 +32,9 @@ export function readDefaultTsConfig(fileName?: string): ng.ParsedConfiguration { | |||
| // these are required to set the appropriate EmitFlags | |||
| flatModuleId: 'AUTOGENERATED', | |||
| flatModuleOutFile: 'AUTOGENERATED', | |||
|
|
|||
| // performance | |||
| skipLibCheck: true, // don't type-check .d.ts files for performance and memory reasons. | |||
There was a problem hiding this comment.
There's also skipDefaultLibCheck which would continue to type-check d.ts files, but that one is deprecated in favor of skipLibCheck itself. See also microsoft/TypeScript#31417 for a relevant discussion on this flag.
There was a problem hiding this comment.
I think we should leave this one out.
This is a small performance improvement in which an ajv validator for the ng-package JSON schema is only compiled once. Additionally, the schema validation logic is refactored into its own file.
JoostK
force-pushed
the
perf
branch
4 times, most recently
from
c358a4d
to
42365ae
Compare
| const cachedSourceFile = sourcesFileCache.get(filePath); | ||
|
|
||
| ngccProcessingCache.clear(); |
There was a problem hiding this comment.
Do we need to clear the cache on each file file? We don't watch node_modules therefore I assume that we shouldn't do this.
There was a problem hiding this comment.
I wanted to avoid a situation where node_modules was somehow affected between watch builds, as in that case the cache would incorrectly cause ngcc not to run. Even if we don't watch node_modules it's still possible for someone to change a file that is watched after something in node_modules has changed.
There was a problem hiding this comment.
I think this will not work because of the "local" processed files cache processedModules
There was a problem hiding this comment.
NgccProcessor is recreated in the transform pipeline, so it's not a persistent cache per entry-point.
| * if all dependencies have been processed. This cache allows to avoid that work, as entry-points | ||
| * are processed in batches during which the `node_modules` directory is not mutated. | ||
| */ | ||
| export class NgccProcessingCache { |
There was a problem hiding this comment.
NIT: Why not simply use a Set directly?
There was a problem hiding this comment.
I like the documentation aspect of having the dedicated class (JS docblock, method names, etc.). It also provides type-safety with respect to other Set instances, so that's nice as well.
There was a problem hiding this comment.
While you have a point with docs for methods which are not added here :P.
I don't think it adds anything from a type-safety perspective, both the below are equity type-safe, unless I am missing something.
const ngccProcessingCache = new Set<string>();
const ngccProcessingCache = new NgccProcessingCache();I don't have a very strong option about this and you can keep it as is. But I do think that in this case using a class doesn't add any value, especially for an internal implementation on top of that which is used in a single place.
Also, while in this case they are not in use, wrapping the Set with a Class has also some disadvantages where you don't expose all the methods and properties available for it. Example, if you need to use .size or entries, you'd need to implement another method like such https://github.com/ng-packagr/ng-packagr/blob/master/src/lib/file-system/file-cache.ts
There was a problem hiding this comment.
What I meant by type-safety is the fact that two Sets can easily be interchanged, which is not the case with two distinct implementations. I phrased the earlier comment quite poorly, hopefully this makes more sense. It's similar to the AbsoluteFsPath branded type that's used in the compiler; although it is a string it does offer identification/documentation and more type-safety compared to just using a string.
While you have a point with docs for methods which are not added here :P.
😅 I was mainly referring to the docblock on the class itself.
I see what you're saying from an internal implementation perspective and the fact that this thing is used in so few places. I still like the insight in brings when just glancing over the code, and e.g. the ability to ask an IDE where things are marked as processed (which wouldn't be as easy if this were just Set).
| * | ||
| * @param ngPackageJson The value to validate. | ||
| */ | ||
| export function validateNgPackageSchema(ngPackageJson: unknown): asserts ngPackageJson is NgPackageConfig { |
This commit introduces a cache of which entry-points have already been processed by ngcc that is used across entry-points, to avoid the overhead of invoking ngcc repeatedly for all entry-points.
As the analysis file cache was previously never invalidated, any newly introduced imports would not be detected in the analysis phase. This could result in e.g. circular imports or self imports to go unnoticed during watch builds.
|
This PR has been automatically locked due to inactivity. |
See individual commits.