Skip to content

Commit

Permalink
perf(core): Update LView consumer to only mark component for check (#…
Browse files Browse the repository at this point in the history
…52302)

This commit updates the reactive template and host binding consumers to
only mark their declaration components for refresh, but not parents/ancestors.

This also updates the `AfterViewChecked` hook to run when a component is
refreshed during change detection but its host is not. It is reasonable
to expect that the `ngAfterViewChecked` lifecycle hook will run when a
signal updates and the component is refreshed. The hooks are typically
run when the host is refreshed so without this change, the update to
not mark ancestors dirty would have caused `ngAfterViewChecked` to not
run.

resolves #14628
resolves #22646

resolves #34347 - this is not the direct request of the issue but
generally forcing change detection to run is necessary only because a
value was updated that needs to be synced to the DOM. Values that use
signals will mark the component for check automatically so accessing the
`ChangeDetectorRef` of a child is not necessary. The other part of this
request was to avoid the need to "mark all views for checking since
it wouldn't affect anything but itself". This is directly addressed by
this commit - updating a signal that's read in the view's template
will not cause ancestors/"all views" to be refreshed.

PR Close #52302
  • Loading branch information
atscott authored and alxhub committed Oct 31, 2023
1 parent e74b578 commit 3861a73
Show file tree
Hide file tree
Showing 5 changed files with 573 additions and 146 deletions.
13 changes: 10 additions & 3 deletions packages/core/src/render3/instructions/change_detection.ts
Expand Up @@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery';
import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView} from '../interfaces/view';
import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
Expand Down Expand Up @@ -344,15 +344,22 @@ function detectChangesInViewIfAttached(lView: LView, mode: ChangeDetectionMode)
* view HasChildViewsToRefresh flag is set.
*/
function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode();
const tView = lView[TVIEW];
const flags = lView[FLAGS];

// Flag cleared before change detection runs so that the view can be re-marked for traversal if
// necessary.
lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView);

if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) &&
mode === ChangeDetectionMode.Global) ||
if ((flags & LViewFlags.CheckAlways && mode === ChangeDetectionMode.Global) ||
(flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global &&
// CheckNoChanges never worked with `OnPush` components because the `Dirty` flag was cleared
// before checkNoChanges ran. Because there is now a loop for to check for backwards views,
// it gives an opportunity for `OnPush` components to be marked `Dirty` before the
// CheckNoChanges pass. We don't want existing errors that are hidden by the current
// CheckNoChanges bug to surface when making unrelated changes.
!isInCheckNoChangesPass) ||
flags & LViewFlags.RefreshView) {
refreshView(tView, lView, tView.template, lView[CONTEXT]);
} else if (flags & LViewFlags.HasChildViewsToRefresh) {
Expand Down
7 changes: 2 additions & 5 deletions packages/core/src/render3/reactive_lview_consumer.ts
Expand Up @@ -8,11 +8,8 @@

import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals';

import {RuntimeError} from '../errors';
import {assertDefined} from '../util/assert';

import {markViewDirty} from './instructions/mark_view_dirty';
import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
import {markViewDirtyFromSignal} from './util/view_utils';

let currentConsumer: ReactiveLViewConsumer|null = null;
export interface ReactiveLViewConsumer extends ReactiveNode {
Expand Down Expand Up @@ -43,7 +40,7 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'|'slot'>
` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` +
` entirely.`);
}
markViewDirty(node.lView);
markViewDirtyFromSignal(node.lView);
},
consumerOnSignalRead(this: ReactiveLViewConsumer): void {
if (currentConsumer !== this) {
Expand Down
21 changes: 20 additions & 1 deletion packages/core/src/render3/util/view_utils.ts
Expand Up @@ -13,7 +13,7 @@ import {HAS_CHILD_VIEWS_TO_REFRESH, LContainer, TYPE} from '../interfaces/contai
import {TConstants, TNode} from '../interfaces/node';
import {RNode} from '../interfaces/renderer_dom';
import {isLContainer, isLView} from '../interfaces/type_checks';
import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';
import {DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';



Expand Down Expand Up @@ -241,6 +241,25 @@ export function markAncestorsForTraversal(lView: LView) {
}
}

/**
* Marks the component or root view of an LView for refresh.
*
* This function locates the declaration component view of a given LView and marks it for refresh.
* With this, we get component-level change detection granularity. Marking the `LView` itself for
* refresh would be view-level granularity.
*
* Note that when an LView is a root view, the DECLARATION_COMPONENT_VIEW will be the root view
* itself. This is a bit confusing since the TView.type is `Root`, rather than `Component`, but this
* is actually what we need for host bindings in a root view.
*/
export function markViewDirtyFromSignal(lView: LView): void {
const declarationComponentView = lView[DECLARATION_COMPONENT_VIEW];
declarationComponentView[FLAGS] |= LViewFlags.RefreshView;
if (viewAttachedToChangeDetector(declarationComponentView)) {
markAncestorsForTraversal(declarationComponentView);
}
}

/**
* Stores a LView-specific destroy callback.
*/
Expand Down

0 comments on commit 3861a73

Please sign in to comment.