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

perf(core): do not recurse into modules that have already been registered #39514

Closed
wants to merge 2 commits into from
Closed

perf(core): do not recurse into modules that have already been registered #39514

wants to merge 2 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Oct 30, 2020

When registering an NgModule based on its id, all transitively imported
NgModules are also registered. This commit introduces a visited set to
avoid traversing into NgModules that are reachable from multiple import
paths multiple times.

Fixes #39487

@JoostK JoostK added type: bug/fix area: performance Issues related to performance state: WIP area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release core: performance labels Oct 30, 2020
@ngbot ngbot bot modified the milestone: needsTriage Oct 30, 2020
@google-cla google-cla bot added the cla: yes label Oct 30, 2020
@JoostK JoostK force-pushed the register-module-perf branch from 6fe6a7f to 4e57dee Compare October 30, 2020 19:56
@JoostK JoostK mentioned this pull request Oct 30, 2020
Closed
@JoostK JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 30, 2020
@JoostK JoostK marked this pull request as ready for review October 30, 2020 20:49
@pullapprove pullapprove bot requested a review from AndrewKushnir October 30, 2020 20:49
AndrewKushnir
AndrewKushnir approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Nice optimization @JoostK 👍 (just left 1 minor comment)

packages/core/src/linker/ng_module_factory_registration.ts Outdated
const visited = new Set<NgModuleType>();
recurse(ngModuleType);
function recurse(ngModuleType: NgModuleType): void {
const def = getNgModuleDef(ngModuleType, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const def = getNgModuleDef(ngModuleType, true);
const def = getNgModuleDef(ngModuleType, /* throwNotFound */ true);

May be also add a quick comment on why it's safe to throw if moduleDef is not found?

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Oct 30, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

@JoostK JoostK added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 30, 2020
JoostK added 2 commits October 31, 2020 00:08
@JoostK
perf(core): do not recurse into modules that have already been regist…
dfaf450 
…ered

When registering an NgModule based on its id, all transitively imported
NgModules are also registered. This commit introduces a visited set to
avoid traversing into NgModules that are reachable from multiple import
paths multiple times.

Fixes angular#39487
@JoostK
fixup! perf(core): do not recurse into modules that have already been…
2b05235 
… registered
@JoostK JoostK changed the title perf(core): do not recursive into modules that have already been registered perf(core): do not recurse into modules that have already been registered Oct 30, 2020
@JoostK JoostK force-pushed the register-module-perf branch from af1412e to 2b05235 Compare October 30, 2020 23:08
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Oct 30, 2020
josephperrott pushed a commit that referenced this pull request Nov 2, 2020
@JoostK @josephperrott
perf(core): do not recurse into modules that have already been regist…
812355c 
…ered (#39514)

When registering an NgModule based on its id, all transitively imported
NgModules are also registered. This commit introduces a visited set to
avoid traversing into NgModules that are reachable from multiple import
paths multiple times.

Fixes #39487

PR Close #39514
josephperrott pushed a commit that referenced this pull request Nov 2, 2020
@JoostK @josephperrott
perf(core): do not recurse into modules that have already been regist…
5c13c67 
…ered (#39514)

When registering an NgModule based on its id, all transitively imported
NgModules are also registered. This commit introduces a visited set to
avoid traversing into NgModules that are reachable from multiple import
paths multiple times.

Fixes #39487

PR Close #39514
@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 Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime area: performance Issues related to performance cla: yes core: performance target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Perfomance issue on module init for big apps caused by registerNgModuleType function.
2 participants
@JoostK @AndrewKushnir