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

View processing create update split #32020

Conversation

@pkozlowski-opensource
Copy link
Member

commented Aug 6, 2019

This PR splits view processing into a set of 2 dedicated functions:

  • renderView (to process a view in the creation mode)
  • refreshView (to process a view in the update mode)

Such split allows us to greatly reduce a number of checks (memory access and computation) for both create and update mode.

This PR has all the commits as they were written to ease review. The essential part (new renderView and refreshView functions) is in the first commit of this PR, the follow up commits are mostly refactors and cleanups (turns out that we can express lots of the existing logic in terms of renderView and refreshView functions).

With all the changes in this PR we get massive perf improvement (~2s -> ~1s in a dedicated benchmark) for view traversal and great reduction in number of concepts / code duplication.

@googlebot googlebot added the cla: yes label Aug 6, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:view_processing_create_update_split branch 3 times, most recently from 107859f to 9f63074 Aug 6, 2019

@ngbot ngbot bot added this to the needsTriage milestone Aug 7, 2019

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Aug 7, 2019

@pkozlowski-opensource pkozlowski-opensource requested review from angular/fw-core as code owners Aug 7, 2019

@mhevery
mhevery approved these changes Aug 8, 2019
Copy link
Member

left a comment

I left some suggestions for future refactorings.

// off firstTemplatePass. If we don't set it here, instances will perform directive
// matching, etc again and again.
if (tView.firstTemplatePass) {
tView.firstTemplatePass = false;

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

I assume it is true that reading is faster than writing and hence guarding this way makes sense. If so can we put that in the PERF.md?

// We must materialize query results before child components are processed
// in case a child component has projected a container. The LContainer needs
// to exist so the embedded views are properly attached by the container.
if (tView.staticViewQueries) {

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

I see that we are reading firstTemplatePass, staticContentQueries and staticViewQueries We could change all of those to a single number read and then use bits as flags. This way we would only have a single read.


const checkNoChangesMode = getCheckNoChangesMode();

executePreOrderHooks(lView, tView, checkNoChangesMode, undefined);

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

if we had a flag which would store if we have pre order hooks that we could guard this. This flag could be stored along with check no changes mode?

refreshDynamicEmbeddedViews(lView);

// Content query results must be refreshed before content hooks are called.
if (tView.contentQueries !== null) {

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

flag guard would decrease number of reads.

refreshContentQueries(tView, lView);
}

resetPreOrderHookFlags(lView);

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

this could be guarded by a hook and not executed.

packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
refreshChildComponents(lView, components);
}

resetPreOrderHookFlags(lView);

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 8, 2019

Member

this could be guarded by a hook and not executed.

@kara kara self-requested a review Aug 8, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:view_processing_create_update_split branch 2 times, most recently from 868d2ee to 67e8d5e Aug 9, 2019

@kara
Copy link
Contributor

left a comment

Love these changes! Happy that we were able to remove so much code duplication during the perf cleanup.

Most of the comments are nits/questions except the one about failing G3 tests

@pkozlowski-opensource pkozlowski-opensource requested a review from angular/fw-i18n as a code owner Aug 13, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:view_processing_create_update_split branch 2 times, most recently from 06ffc58 to 690e8f7 Aug 13, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:view_processing_create_update_split branch from 690e8f7 to 92aa71c Aug 13, 2019

@kara
kara approved these changes Aug 13, 2019
Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@kara kara closed this in b9dfe66 Aug 13, 2019

sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Sep 15, 2019

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.
Projects
None yet
5 participants
You can’t perform that action at this time.