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.

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

perf(ivy): R3TestBed - Do not process NgModuleDefs that have already … #33863

Closed
wants to merge 3 commits into from

Conversation

@atscott
Copy link
Contributor

atscott commented Nov 15, 2019

…been processed

I was observing 10x slower test times in Ivy vs VE and this change made Ivy perform as fast as VE. Perf analysis showed that 96% of the time was spent in queueTypesFromModulesArray. This can happen with deep trees of imports that have duplicated NgModule imports throughout.

@googlebot googlebot added the cla: yes label Nov 15, 2019
@AndrewKushnir AndrewKushnir self-assigned this Nov 16, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 16, 2019

Thanks for the fix @atscott 👍

I know PR is still in Draft mode, but I'd propose adding some comments on why we do caching (since we may encounter the same NgModules while going though imports and exports, so we want to avoid re-processing). Otherwise - looks great!

@atscott atscott requested a review from AndrewKushnir Nov 18, 2019
@atscott atscott marked this pull request as ready for review Nov 18, 2019
@atscott atscott requested a review from angular/fw-core as a code owner Nov 18, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 18, 2019
atscott added 3 commits Nov 15, 2019
@atscott atscott force-pushed the atscott:testbedperf branch from 911d17e to d78e8d2 Nov 19, 2019
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Nov 19, 2019

@alxhub alxhub closed this in 5af3bd4 Nov 20, 2019
alxhub added a commit that referenced this pull request Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.