From c83f7cab531b5b2636084244b6db97979a89a3bc Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 8 Feb 2024 09:12:06 -0800 Subject: [PATCH] perf(core): Avoid traversal when `markForCheck` is called on a view being refreshed This commit updates the internal `markViewDirty` logic to avoid marking views dirty up to the root when we encounter a view that is already being refreshed. This is wasteful because we clear the dirty bit at the end of refreshing a view anyways so it really has no affect on that view. This change should reduce the number of tree traversals for replay observables and async pipe. For these cases, when we execute change detection on the view for the first time, the async pipe subscribes to the observable and its value emits synchronously. `markForCheck` is then called inside `AsyncPipe`, which traverses all the way up to the root view. This happens for _every_ `AsyncPipe` binding that emits synchronously on subscription. Sometimes we can reach a view and refresh it when the parents have been skipped. For example, this happens with views that have updated signals. Ancestors are only traversed and not refreshed. Marking dirty all the way up to the root in this case can result in those ancestor views being marked with the dirty flag and it not being cleared. This only became problematic when we started to support backwards referenced transplanted views and changed signal change detection to not mark ancestors for check. In addition to that, the loop in change detection support to remove `ExpressionChanged...` errors for these cases can potentially cause infinite loops in rare cases when these views are refreshed at a time when parents have only been traversed and `markForCheck` is called. Prior to the loop, if this `markForCheck` was combined with an actual change to binding values, this would cause `ExpressionChangedAfterItWasCheckedError`. For regular cases where we are refreshing a view, update a binding, and call `markForCheck`, this is either `ExpressionChanged...` in views with default change detection or no error at all (but without any state update) in views that are `OnPush` since the `Dirty` flag is cleared at the end of `refreshView` and `checkNoChanges` uses the same `Dirty` bit for traversal (#45612). The only case where this might cause different and unexpected behavior is if change detection starts in the middle of a view tree via `ChangeDetectorRef.detectChanges` _and_ a state update happens where the state is read in a view above the root of the traversal along with `markForCheck`. This might be the case for updating host bindings during change detection. That said, we have never really intended to support updating state during change detection, especially in a location above the current spot in the current view tree. This violates unidirectional data flow, a principle we have always intended to enforce outside of using signals for state. --- packages/core/src/application/application_ref.ts | 5 +---- .../src/render3/instructions/change_detection.ts | 2 ++ .../core/src/render3/instructions/mark_view_dirty.ts | 12 ++++++++---- packages/core/src/render3/interfaces/view.ts | 7 ++++++- .../core/test/acceptance/after_render_hook_spec.ts | 3 +-- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index 93197ff6d284..71493e5ddc63 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -702,8 +702,5 @@ export function whenStable(applicationRef: ApplicationRef): Promise { function shouldRecheckView(view: LView): boolean { - return requiresRefreshOrTraversal(view); - // TODO(atscott): We need to support rechecking views marked dirty again in afterRender hooks - // in order to support the transition to zoneless. b/308152025 - /* || !!(view[FLAGS] & LViewFlags.Dirty); */ + return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty); } diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 8dfae64622d3..755e97696e29 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -134,6 +134,7 @@ export function refreshView( !isInCheckNoChangesPass && lView[ENVIRONMENT].inlineEffectRunner?.flush(); + lView[FLAGS] |= LViewFlags.ExecutingRefresh; // Start component reactive context // - We might already be in a reactive context if this is an embedded view of the host. // - We might be descending into a view that needs a consumer. @@ -277,6 +278,7 @@ export function refreshView( maybeReturnReactiveLViewConsumer(currentConsumer); } leaveView(); + lView[FLAGS] &= ~LViewFlags.ExecutingRefresh; } } diff --git a/packages/core/src/render3/instructions/mark_view_dirty.ts b/packages/core/src/render3/instructions/mark_view_dirty.ts index 6f5355cacdd0..54448bed0e8c 100644 --- a/packages/core/src/render3/instructions/mark_view_dirty.ts +++ b/packages/core/src/render3/instructions/mark_view_dirty.ts @@ -19,19 +19,23 @@ import {getLViewParent} from '../util/view_utils'; * an additional time to get the root view and schedule a tick on it. * * @param lView The starting LView to mark dirty - * @returns the root LView */ -export function markViewDirty(lView: LView): LView|null { +export function markViewDirty(lView: LView) { lView[ENVIRONMENT].changeDetectionScheduler?.notify(); while (lView) { + // If we're already refreshing the view, we should not mark it or ancestors dirty. + // This is `ExpressionHasChangedAfterItWasCheckedError` if any bindings have actually changed + // and completely unnecessary to refresh views again if there weren't any updated bindings. + if (lView[FLAGS] & LViewFlags.ExecutingRefresh) { + return; + } lView[FLAGS] |= LViewFlags.Dirty; const parent = getLViewParent(lView); // Stop traversing up as soon as you find a root view that wasn't attached to any container if (isRootView(lView) && !parent) { - return lView; + return; } // continue otherwise lView = parent!; } - return null; } diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 89ea1f82cd62..e8706217d667 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -442,10 +442,15 @@ export const enum LViewFlags { */ HasChildViewsToRefresh = 1 << 13, + /** + * Flag used to indicate that we are current running `refreshView` on the LView. + */ + ExecutingRefresh = 1 << 14, + /** * This is the count of the bits the 1 was shifted above (base 10) */ - IndexWithinInitPhaseShift = 14, + IndexWithinInitPhaseShift = 15, /** * Index of the current init phase on last 21 bits diff --git a/packages/core/test/acceptance/after_render_hook_spec.ts b/packages/core/test/acceptance/after_render_hook_spec.ts index 8568b28db1af..584f4af825b8 100644 --- a/packages/core/test/acceptance/after_render_hook_spec.ts +++ b/packages/core/test/acceptance/after_render_hook_spec.ts @@ -946,8 +946,7 @@ describe('after render hooks', () => { const appRef = TestBed.inject(ApplicationRef); appRef.attachView(fixture.componentRef.hostView); appRef.tick(); - // TODO(atscott): We need to support this for zoneless to succeed - expect(fixture.nativeElement.innerText).toBe('0'); + expect(fixture.nativeElement.innerText).toBe('1'); }); it('throws error when causing infinite updates', () => {