Skip to content

perf(core): Stop marking dirty to the root when a detached view is found #52999

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
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

perf(core): Stop marking dirty to the root when a detached view is found #52999

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Nov 17, 2023

With this change, detached views no longer cause their parents to be marked dirty and subsequently refreshed.

This is a small optimization to stop marking parent views dirty when a detached view is encountered. When running change detection, we stop traversing to children when the view is detached. This means a detached view isn't even reachable from parents so it's wasteful to mark them dirty.

There is a risk, albeit very small, that this could exacerbate the issue described in #52928 where attaching a dirty view does not mark parents dirty. This would be the case if a detached view is marked dirty and then synchronously attached again. Regardless, it may be prudent to address #52928 first or at the same time.

Green TGP other than one unrelated build failure.

@atscott
perf(core): Stop marking dirty to the root when a detached view is found
5b17274 
With this change, detached views no longer cause their parents to be
marked dirty and subsequently refreshed.

This is a small optimization to stop marking parent views dirty when a
detached view is encountered. When running change detection, we stop
traversing to children when the view is detached. This means a detached view
isn't even reachable from parents so it's wasteful to mark them dirty.

There is a risk, albeit _very_ small, that this could exacerbate the
issue described in angular#52928 where attaching a dirty view does not mark
parents dirty. This would be the case if a detached view is marked
dirty and then synchronously attached again. Regardless, it may be
prudent to address angular#52928 first or at the same time.
@atscott
Copy link
Contributor Author

atscott commented Dec 7, 2023 •
edited
Loading

Edit: The concerns below were based thoughts that this PR would stop marking up to the root when an ancestor that was already dirty was found. This PR doesn't do that -- it only stops when a detached ancestor is found. I don't have any concerns other than the one noted in the commit description, which is blocked on the main branch targeting v18.

Thinking about this more, I have concerns with this approach. Failing to mark all the way to the root can turn an ExpressionChangedAfterCheckedError from a small mistake to an unrecoverable breakage. If a view is marked dirty during change detection in a subtree that was already passed over, it will stop when it reaches the parent that was already checked.

This is an ExpressionChanged... error, but now the view will be stuck in a state that will never get updated unless that ancestor gets marked Dirty again somewhere else. Subsequent calls to markForCheck anywhere in the orphaned subtree will exit immediately because all the views have the Dirty flag already.

This problem does not exist with the approach used for signals. The flags are cleared before entering a view during change detection. So there is no ExpressionChanged... error and a subtree that's passed over and then dirtied during change detection later can still re-dirty it's ancestors and be found in another change detection pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@atscott