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.

Sign up for GitHub

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

Jump to bottom

Ngcc perf improvements #30525

Closed
wants to merge 5 commits into from
Closed

Ngcc perf improvements #30525

wants to merge 5 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented May 16, 2019 •
edited

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 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 as code owners 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
Copy link

ngbot bot commented Jul 3, 2019

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
fix(ivy): ngcc - remove unwanted logging message
464149b 
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.
@gkalpak gkalpak mentioned this pull request Jul 6, 2019
Closed
@petebacondarwin
perf(ivy): ngcc - add a cache to the FileSystem
4085252 
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.
@petebacondarwin
perf(ivy): ngcc - only find dependencies when targeting a single entr…
d9bdfd5 
…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.
@petebacondarwin
fix(ivy): ngcc - handle pathMappings to files rather then directories
a8d3e0f 
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
gkalpak
gkalpak approved these changes Jul 8, 2019
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

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 👍

packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts Outdated Show resolved Hide resolved
@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
Member 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
Member Author

Merge assistance - please run presubmit.

@jasonaden
Copy link
Contributor

Presubmit

@jasonaden jasonaden closed this in aaaeb92 Jul 9, 2019
jasonaden pushed a commit that referenced this pull request Jul 9, 2019
@petebacondarwin @jasonaden
perf(ivy): ngcc - add a cache to the FileSystem (#30525)
7f2330a 
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
@petebacondarwin @jasonaden
perf(ivy): ngcc - only find dependencies when targeting a single entr…
a581773 
…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
@petebacondarwin @jasonaden
fix(ivy): ngcc - handle pathMappings to files rather then directories (…
207f9b6 
…#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
@petebacondarwin petebacondarwin mentioned this pull request Jul 15, 2019
Closed
@angular-automatic-lock-bot
Copy link

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

@JoostK JoostK JoostK left review comments

@filipesilva filipesilva filipesilva left review comments

@gkalpak gkalpak gkalpak approved these changes

@alan-agius4 alan-agius4 alan-agius4 left review comments

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
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
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@petebacondarwin @alxhub @jasonaden @JoostK @filipesilva @gkalpak @alan-agius4 @googlebot