Skip to content

Ngcc perf improvements - #30525

Closed
petebacondarwin wants to merge 5 commits into
angular:masterfrom
petebacondarwin:ngcc-perf-improvements
Closed

petebacondarwin wants to merge 5 commits into
angular:masterfrom
petebacondarwin:ngcc-perf-improvements

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented May 16, 2019 •

Copy link
Copy Markdown
Contributor

This PR sits on top of #30591

@jasonaden jasonaden added the area: core Issues related to the framework runtime label May 17, 2019
@ngbot ngbot Bot added this to the needsTriage milestone May 17, 2019
@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch from b5829b7 to f3a5dcd Compare May 18, 2019 09:21
@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch 2 times, most recently from f336991 to 402c568 Compare May 29, 2019 06:59
@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch 6 times, most recently from c29e486 to dca67d6 Compare June 24, 2019 14:35
@petebacondarwin
petebacondarwin marked this pull request as ready for review June 24, 2019 14:47
@petebacondarwin
petebacondarwin requested review from a team June 24, 2019 14:47
@petebacondarwin petebacondarwin added area: compiler Issues related to `ngc`, Angular's template compiler comp: ivy effort2: days freq2: medium state: blocked target: major This PR is targeted for the next major release risk: medium refactoring Issue that involves refactoring or code-cleanup labels Jun 24, 2019
@ngbot ngbot Bot modified the milestones: needsTriage, Backlog Jun 24, 2019
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Jul 3, 2019
@ngbot

ngbot Bot commented Jul 3, 2019

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci-codefresh-bazel" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch from 2e711d3 to c118abb Compare July 3, 2019 19:58
This message gets called if a format has already been
compiled and we only want the first. So the message itself
is wrong but it is also not very useful anyway.
@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch from c118abb to 5c23da4 Compare July 4, 2019 10:28
When profiling ngcc it is notable that a large amount of time
is spent dealing with an exception that is thrown (and handled
internally by fs) when checking the existence of a file.

We check file existence a lot in both finding entry-points
and when TS is compiling code. This commit adds a simple
cached `FileSystem`, which wraps a real `FileSystem` delegate.
This will reduce the number of calls through to `fs.exists()` and
`fs.readFile()` on the delegate.

Initial benchmarks indicate that the cache is miss to hit ratio
for `exists()` is about 2:1, which means that we save about 1/3
of the calls to `fs.existsSync()`.

Note that this implements a "non-expiring" cache, so it is not suitable
for a long lived `FileSystem`, where files may be modified externally.
The cache will be updated if a file is changed or moved via
calls to `FileSystem` methods but it will not be aware of changes
to the files system from outside the `FileSystem` service.

For ngcc we must create a new `FileSystem` service
for each run of `mainNgcc` and ensure that all file operations
(including TS compilation) use the `FileSystem` service.
This ensures that it is very unlikely that a file will change
externally during `mainNgcc` processing.
…y-point

Previously, ngcc had to walk the entire `node_modules` tree looking for
entry-points, even if it only needed to process a single target entry-point
and its dependencies.

This added up to a few seconds to each execution of ngcc, which is noticeable
when being run via the CLI integration.

Now, if an entry-point target is provided, only that target and its entry-points
are considered rather than the whole folder tree.
Paths can be mapped directly to files, which was not being taken
into account when computing `basePaths` for the `EntryPointFinder`s.

Now if a `pathMapping` pattern does not exist or is a file, then we try
the containing folder instead.

Fixes angular#31424
@petebacondarwin
petebacondarwin force-pushed the ngcc-perf-improvements branch from d94b8a2 to a8d3e0f Compare July 8, 2019 09:58

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to have a test with a path mapped to a parent directory (something like paths: {'@foo': ['../foo']}) to make sure .. segments are resolved correctly.

Otherwise lgtm 👍

@gkalpak gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 8, 2019
@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Added a test.

@petebacondarwin petebacondarwin added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 8, 2019
@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Merge assistance - please run presubmit.

@jasonaden

Copy link
Copy Markdown
Contributor

Presubmit

@jasonaden jasonaden closed this in aaaeb92 Jul 9, 2019
jasonaden pushed a commit that referenced this pull request Jul 9, 2019
When profiling ngcc it is notable that a large amount of time
is spent dealing with an exception that is thrown (and handled
internally by fs) when checking the existence of a file.

We check file existence a lot in both finding entry-points
and when TS is compiling code. This commit adds a simple
cached `FileSystem`, which wraps a real `FileSystem` delegate.
This will reduce the number of calls through to `fs.exists()` and
`fs.readFile()` on the delegate.

Initial benchmarks indicate that the cache is miss to hit ratio
for `exists()` is about 2:1, which means that we save about 1/3
of the calls to `fs.existsSync()`.

Note that this implements a "non-expiring" cache, so it is not suitable
for a long lived `FileSystem`, where files may be modified externally.
The cache will be updated if a file is changed or moved via
calls to `FileSystem` methods but it will not be aware of changes
to the files system from outside the `FileSystem` service.

For ngcc we must create a new `FileSystem` service
for each run of `mainNgcc` and ensure that all file operations
(including TS compilation) use the `FileSystem` service.
This ensures that it is very unlikely that a file will change
externally during `mainNgcc` processing.

PR Close #30525
jasonaden pushed a commit that referenced this pull request Jul 9, 2019
…y-point (#30525)

Previously, ngcc had to walk the entire `node_modules` tree looking for
entry-points, even if it only needed to process a single target entry-point
and its dependencies.

This added up to a few seconds to each execution of ngcc, which is noticeable
when being run via the CLI integration.

Now, if an entry-point target is provided, only that target and its entry-points
are considered rather than the whole folder tree.

PR Close #30525
jasonaden pushed a commit that referenced this pull request Jul 9, 2019
…#30525)

Paths can be mapped directly to files, which was not being taken
into account when computing `basePaths` for the `EntryPointFinder`s.

Now if a `pathMapping` pattern does not exist or is a file, then we try
the containing folder instead.

Fixes #31424

PR Close #30525
@petebacondarwin
petebacondarwin deleted the ngcc-perf-improvements branch July 10, 2019 09:42
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime cla: yes effort2: days freq2: medium merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note refactoring Issue that involves refactoring or code-cleanup risk: medium target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants