ngcc: implement a new program-based entry-point finder #37075

petebacondarwin · 2020-05-12T18:36:38Z

Add a new EntryPointFinder that can be seeded from the imports in the program specified by a tsconfig.json file. This should be much faster than the DirectoryWalkerEntryPointFinder when the active program only imports a small proportion of the installed entry-points.

On the AIO project only half the entry-points would be processed by ngcc if the CLI async integration with ngcc made use of the new flag.

petebacondarwin · 2020-05-20T16:35:26Z

The approach here, which involves parsing each source file, could be slow for a large app.
We should investigate using the TS scanner directly or even a simple regular expression, since we only care about capturing the import paths and can be quite tolerant of false positives.

JoostK

Typo in "capture dynamic import expressions as well as declarations" commit:

"and creates lesss" -> "and creates less"

packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts

packages/compiler-cli/ngcc/test/dependencies/esm_dependency_host_spec.ts

packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts

packages/compiler-cli/ngcc/src/command_line_options.ts

packages/compiler-cli/ngcc/test/entry_point_finder/program_based_entry_point_finder_spec.ts

The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close #37075

…#37075) Previously we only checked for static import declaration statements. This commit also finds import paths from dynamic import expressions. Also this commit should speed up processing: Previously we were parsing the source code contents into a `ts.SourceFile` and then walking the parsed AST to find import paths. Generating an AST is unnecessary work and it is faster and creates less memory pressure to just scan the source code contents with the TypeScript scanner, identifying import paths from the tokens. PR Close #37075

This finder is designed to only process entry-points that are reachable by the program defined by a tsconfig.json file. It is triggered by calling `mainNgcc()` with the `findEntryPointsFromTsConfigProgram` option set to true. It is ignored if a `targetEntryPointPath` has been provided as well. It is triggered from the command line by adding the `--use-program-dependencies` option, which is also ignored if the `--target` option has been provided. Using this option can speed up processing in cases where there is a large number of dependencies installed but only a small proportion of the entry-points are actually imported into the application. PR Close #37075

The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close #37075

…#37075) Previously we only checked for static import declaration statements. This commit also finds import paths from dynamic import expressions. Also this commit should speed up processing: Previously we were parsing the source code contents into a `ts.SourceFile` and then walking the parsed AST to find import paths. Generating an AST is unnecessary work and it is faster and creates less memory pressure to just scan the source code contents with the TypeScript scanner, identifying import paths from the tokens. PR Close #37075

This finder is designed to only process entry-points that are reachable by the program defined by a tsconfig.json file. It is triggered by calling `mainNgcc()` with the `findEntryPointsFromTsConfigProgram` option set to true. It is ignored if a `targetEntryPointPath` has been provided as well. It is triggered from the command line by adding the `--use-program-dependencies` option, which is also ignored if the `--target` option has been provided. Using this option can speed up processing in cases where there is a large number of dependencies installed but only a small proportion of the entry-points are actually imported into the application. PR Close #37075

gkalpak

Post-merge review 😁:
tl;dr LGTM 👍

gkalpak · 2020-06-05T10:49:06Z

packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts

+            imports.add(reexportPath);
+          }
+          break;
+      }


This block would benefit from some comments explaining what each case does and why. It is quite hard (for me) to understand without diving into the Scanner.

gkalpak · 2020-06-05T10:49:48Z

packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts

+      lastToken = currentToken;
+    }
+
+    // Clear the text from the scanner.


gkalpak · 2020-06-05T14:31:22Z

packages/compiler-cli/ngcc/src/dependencies/esm_dependency_host.ts

@@ -93,7 +225,7 @@ export class EsmDependencyHost extends DependencyHostBase {
 * in this file, true otherwise.
 */
 export function hasImportOrReexportStatements(source: string): boolean {
-  return /(import|export)\s.+from/.test(source);
+  return /(?:import|export)[\s\S]+?(["'])(?:(?:\\\1|.)*?)\1/.test(source);


Nice 👍

There is one unneeded set of parenthesis (afaict), but not really important:

Suggested change

return /(?:import|export)[\s\S]+?(["'])(?:(?:\\\1|.)*?)\1/.test(source);

return /(?:import|export)[\s\S]+?(["'])(?:\\\1|.)+?\1/.test(source);

I'm a bit frightened to touch that again :-)

gkalpak · 2020-06-05T14:39:22Z

packages/compiler-cli/ngcc/src/entry_point_finder/tracing_entry_point_finder.ts

+ * This is faster than searching the entire file-system for all the entry-points,
+ * and is used primarily by the CLI integration.
+ *
+ * There are two concrete implementation of this class.


Suggested change

* There are two concrete implementation of this class.

* There are two concrete implementations of this class.

petebacondarwin · 2020-06-05T15:41:30Z

Thanks for the extra review @gkalpak - here is a fixup #37461

This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged.

…pos (#37040) This is a follow-up to #37075, because I didn't manage to finish my review before the PR got merged. PR Close #37040

…ost` (angular#37075) Previously this host was skipping files if they had imports that spanned multiple lines, or if the import was a dynamic import expression. PR Close angular#37075

…7075) The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close angular#37075

…angular#37075) Previously we only checked for static import declaration statements. This commit also finds import paths from dynamic import expressions. Also this commit should speed up processing: Previously we were parsing the source code contents into a `ts.SourceFile` and then walking the parsed AST to find import paths. Generating an AST is unnecessary work and it is faster and creates less memory pressure to just scan the source code contents with the TypeScript scanner, identifying import paths from the tokens. PR Close angular#37075

This finder is designed to only process entry-points that are reachable by the program defined by a tsconfig.json file. It is triggered by calling `mainNgcc()` with the `findEntryPointsFromTsConfigProgram` option set to true. It is ignored if a `targetEntryPointPath` has been provided as well. It is triggered from the command line by adding the `--use-program-dependencies` option, which is also ignored if the `--target` option has been provided. Using this option can speed up processing in cases where there is a large number of dependencies installed but only a small proportion of the entry-points are actually imported into the application. PR Close angular#37075

…pos (angular#37040) This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged. PR Close angular#37040

With this change we consume the change in angular/angular#37075 where the NGCC now exposes a new option `--use-program-dependencies`, to only process packages which are part of the TypeScript program. From the initial benchmarking the time taken for NGCC to process that dependencies needed for an `ng new` application went down by 36% from 10526ms to 6708ms.

angular-automatic-lock-bot · 2020-07-06T16:13:35Z

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.}

…ost` (angular#37075) Previously this host was skipping files if they had imports that spanned multiple lines, or if the import was a dynamic import expression. PR Close angular#37075

…7075) The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close angular#37075

…angular#37075) Previously we only checked for static import declaration statements. This commit also finds import paths from dynamic import expressions. Also this commit should speed up processing: Previously we were parsing the source code contents into a `ts.SourceFile` and then walking the parsed AST to find import paths. Generating an AST is unnecessary work and it is faster and creates less memory pressure to just scan the source code contents with the TypeScript scanner, identifying import paths from the tokens. PR Close angular#37075

This finder is designed to only process entry-points that are reachable by the program defined by a tsconfig.json file. It is triggered by calling `mainNgcc()` with the `findEntryPointsFromTsConfigProgram` option set to true. It is ignored if a `targetEntryPointPath` has been provided as well. It is triggered from the command line by adding the `--use-program-dependencies` option, which is also ignored if the `--target` option has been provided. Using this option can speed up processing in cases where there is a large number of dependencies installed but only a small proportion of the entry-points are actually imported into the application. PR Close angular#37075

googlebot added the cla: yes label May 12, 2020

petebacondarwin added comp: ngcc state: WIP feature Issue that requests a new feature labels May 12, 2020

ngbot bot modified the milestone: needsTriage May 12, 2020

gkalpak mentioned this pull request May 22, 2020

Incorrect bundles and main paths in package.json since v11 carbon-design-system/carbon-icons-angular#15

Closed

This was referenced May 24, 2020

Upgrade to 9.1 - hangs on 0% compiling #36272

Closed

Slower Build Performance after Upgrade from 9.0.7 to 9.1.0 #36414

Closed

CICD builds are very slow. Please provide compile caching #37297

Closed

petebacondarwin force-pushed the ngcc-target-program branch 4 times, most recently from 562fc7e to 87c7485 Compare June 2, 2020 18:35

petebacondarwin changed the title ~~feat(ngcc): WIP - implement a new program-based entry-point finder~~ feat(ngcc): implement a new program-based entry-point finder Jun 2, 2020

petebacondarwin force-pushed the ngcc-target-program branch 3 times, most recently from 423b945 to 2754a92 Compare June 3, 2020 15:13

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: performance and removed state: WIP feature Issue that requests a new feature labels Jun 3, 2020

petebacondarwin marked this pull request as ready for review June 3, 2020 15:13

pullapprove bot requested review from alxhub and gkalpak June 3, 2020 15:13

JoostK reviewed Jun 3, 2020

View reviewed changes

petebacondarwin changed the title ~~feat(ngcc): implement a new program-based entry-point finder~~ ngcc: implement a new program-based entry-point finder Jun 3, 2020

petebacondarwin force-pushed the ngcc-target-program branch from 2754a92 to 300fde5 Compare June 3, 2020 21:41

petebacondarwin requested a review from JoostK June 3, 2020 21:42

atscott pushed a commit that referenced this pull request Jun 4, 2020

refactor(ngcc): move shared code into DependencyHostBase (#37075)

8389025

The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close #37075

atscott closed this in 7f98b87 Jun 4, 2020

atscott pushed a commit that referenced this pull request Jun 4, 2020

refactor(ngcc): move shared code into DependencyHostBase (#37075)

4d69da5

The various dependency hosts had a lot of duplicated code. This commit refactors them to move this into the base class. PR Close #37075

petebacondarwin deleted the ngcc-target-program branch June 4, 2020 18:40

gkalpak reviewed Jun 5, 2020

View reviewed changes

gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 5, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, typos

9ef5c61

This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged.

gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 8, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, typos

120a49a

This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged.

gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 8, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, typos

9dbbaf2

This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged.

alan-agius4 mentioned this pull request Jun 9, 2020

perf(@ngtools/webpack): only process dependent packages with NGCC angular/angular-cli#17884

Merged

gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 10, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, typos

6cac7ff

This is a follow-up to angular#37075, because I didn't manage to finish my review before the PR got merged.

mhevery pushed a commit that referenced this pull request Jun 12, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, ty…

ab9bc8a

…pos (#37040) This is a follow-up to #37075, because I didn't manage to finish my review before the PR got merged. PR Close #37040

mhevery pushed a commit that referenced this pull request Jun 12, 2020

refactor(ngcc): clean up unused imports, unused regex parenthesis, ty…

a32579a

…pos (#37040) This is a follow-up to #37075, because I didn't manage to finish my review before the PR got merged. PR Close #37040

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngcc: implement a new program-based entry-point finder #37075

ngcc: implement a new program-based entry-point finder #37075

petebacondarwin commented May 12, 2020 •

edited

petebacondarwin commented May 20, 2020

JoostK left a comment

gkalpak left a comment

gkalpak Jun 5, 2020

gkalpak Jun 5, 2020

gkalpak Jun 5, 2020

petebacondarwin Jun 5, 2020

gkalpak Jun 5, 2020

petebacondarwin commented Jun 5, 2020

angular-automatic-lock-bot bot commented Jul 6, 2020

	return /(?:import\|export)[\s\S]+?(["'])(?:(?:\\\1\|.)*?)\1/.test(source);
	return /(?:import\|export)[\s\S]+?(["'])(?:\\\1\|.)+?\1/.test(source);

	* There are two concrete implementation of this class.
	* There are two concrete implementations of this class.

ngcc: implement a new program-based entry-point finder #37075

ngcc: implement a new program-based entry-point finder #37075

Conversation

petebacondarwin commented May 12, 2020 • edited

petebacondarwin commented May 20, 2020

JoostK left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Jun 5, 2020

Choose a reason for hiding this comment

gkalpak Jun 5, 2020

Choose a reason for hiding this comment

gkalpak Jun 5, 2020

Choose a reason for hiding this comment

petebacondarwin Jun 5, 2020

Choose a reason for hiding this comment

gkalpak Jun 5, 2020

Choose a reason for hiding this comment

petebacondarwin commented Jun 5, 2020

angular-automatic-lock-bot bot commented Jul 6, 2020

petebacondarwin commented May 12, 2020 •

edited