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

Merge dirty relayout boundaries after RenderObject.invokeLayoutCallback #105175

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Jun 1, 2022

An example where widget 3's subtree will be laid out twice in one frame:

InheritedWidget
  |
LayoutBuilder with an anonymous function as builder 
  |
Widget 1, which prevents incoming constraints from forcing a tree walk 
  |
Widget 2 which produces a relayout boundary
  |
Widget 3, which produces a relayout boundary, and also depends on the InheritedWidget to do layout correctly.

When the InheritedWidget changes, widget 3's render object will be marked as dirty so the initial dirty list will be:
[RenderLayoutBuilder, RenderWidget3].

After PipelineOwner.flushLayout starts, the LayoutBuilder rebuilds, dirtying Widget 2 and causing it to produce different constraints for widget 3. But since
PipelineOwner.flushLayout is already running so the list of dirty relayout boundaries is already locked. So RenderWidget3 still gets to do layout before RenderWidget2, using outdated constraints.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 1, 2022
@LongCatIsLooong LongCatIsLooong changed the title Merge dirty relayout boundaries after [RenderObject.invokeLayoutCallback] Merge dirty relayout boundaries after RenderObject.invokeLayoutCallback Jun 1, 2022
if (node._needsLayout && node.owner == this) {
node._layoutWithoutResize();
}
}
}
} finally {
_shouldMergeDirtyNodes = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the node causing _shouldMergeDirtyNodes to be set to true is the last node in dirtyNodes this could cause an unnecessary node merging, no?

Scenario: Line 1000 causes _shouldMergeDirtyNodes to go to true and since there are no more nodes in dirtyNodes, we exit the for loop. Since there are new node in _nodesNeedingLayout we do another run through the body of the while: We sort the nodes, enter the for loop again and since _shouldMergeDirtyNodes is still true, we do an unnecessary node merge.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fluttergithubbot fluttergithubbot merged commit d378515 into flutter:master Jun 3, 2022
@LongCatIsLooong LongCatIsLooong deleted the merge-dirty-nodes-after-invokeLayoutCallback branch June 3, 2022 02:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants