Document RenderObject._relayoutBoundary and its invariant; small refactors #150527
Conversation
This concept was renamed in 4e3e40a (flutter#4483), back in 2016, so for example _relayoutSubtreeRoot became _relayoutBoundary. This method _debugSubtreeRelayoutRootAlreadyMarkedNeedsLayout was left behind in that change, though -- perhaps because it had the words in a different order. Fix the name now, in order to align with the related other names.
This is an important invariant for understanding how _needsLayout relates to the methods that do layout, and to related flags like _debugMutationsLocked and _debugDoingThisLayout. The invariant can be verified by reading the one call site of this private method. But that's about 1300 lines away, so it's helpful to assert it here locally in this method it concerns.
This is a pure refactor.
This is a small pure refactor, which will simplify describing the invariant that this code maintains on `_relayoutBoundary`.
The "same as its parent" form of the invariant can be verified by inspecting all the places _relayoutBoundary or _parent are modified. Because those fields are private, those are all in this file; and there are only a handful of them. The "ancestor" form of the invariant can be verified by induction using the "same as its parent" form.
…outBoundary Doing these updates in a post-order traversal rather than pre-order is a pure refactor, but simplifies reasoning about the invariant by making it remain true at every step.
This method can only be called as part of a _setRelayoutBoundary call on this node's parent. The invariant on [_relayoutBoundary] means that that property's value here is either (a) null; (b) `this`; or (c) the value that `parent!._relayoutBoundary!` had at the start of that _setRelayoutBoundary call (which may have temporarily broken the invariant). In each of those three cases, this condition will either be true or not be reached. So we can remove it, leaving behind an assertion.
This is a pure refactor that just reduces the number of separate yet mutually recursive methods involved here, in order to make the code a bit easier to understand.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
LGTM. Thanks for the clean-up.
@LongCatIsLooong since you have looked at layout boundaries before, can you also take a look at this?
| /// | ||
| /// This is set in [layout], and consulted by [markNeedsLayout] in deciding | ||
| /// whether to recursively mark the parent as also needing layout. | ||
| // TODO(gnprice): Just when is _relayoutBoundary null, and what does that mean? |
There was a problem hiding this comment.
I believe null just means the relayout boundary is currently not known. This can happen when the RO is currently not part of the tree. During layout, every RO will be assigned a relayout boundary, though.
There was a problem hiding this comment.
Thanks, this was a good prompt for me to go try to take care of this TODO now. I've just pushed an added commit that replaces it with this paragraph:
/// This property is initially null, and becomes null again if this
/// render object is removed from the tree (with [dropChild]);
/// it remains null until the first layout of this render object
/// after it was most recently added to the tree.
/// This property can also be null while an ancestor in the tree is
/// currently doing layout, until this render object itself does layout.I initially wrote just the first sentence, which is a way of saying much the same thing as your comment, because that sounded right to me. Then I figured I should fact-check it to make sure, so searched again through the places the property gets set to null… and was reminded there's one more wrinkle to it, namely the _visitChildren(_cleanChildRelayoutBoundary) call in layout. (Which must have been part of why I left this as a TODO in the first place.)
But then I added the second sentence, and I think that covers it well enough to cut the TODO.
That particular wrinkle is one that will go away later in my series for #150524, so that'll happily make this a bit cleaner to explain.
|
@gnprice you'll also have to get that test exception. |
|
Thanks for the review! I just bumped my request for that exemption. |
There was a problem hiding this comment.
LGTM.
However to me it feels a lot easier to think of relayout boudaries as special nodes in the tree that qualify for certain optimizations (i.e. from markNeedsLayout's perspective) rather than a set of invariants that need to be maintained by each node in the render tree (from layout's perspective).
| /// Set [_relayoutBoundary] to null throughout this render object's subtree, | ||
| /// stopping at relayout boundaries. | ||
| // This is a static method to reduce closure allocation with visitChildren. | ||
| static void _cleanChildRelayoutBoundary(RenderObject child) { |
There was a problem hiding this comment.
How does this reduce closure allocation? The original form doesn't seem to be called in an anonymous function?
There was a problem hiding this comment.
oh ok looks like the instance method was removed so it makes sense the comment is moved here
| @@ -2185,6 +2185,24 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge | |||
| } | |||
| bool _needsLayout = true; | |||
|
|
|||
| /// The nearest relayout boundary enclosing this render object, if known. | |||
There was a problem hiding this comment.
nit: cross reference [markNeedsLayout] for what a "relayout boundary" is? I feel the text below doesn't explain that.
There was a problem hiding this comment.
or say that a relayout boundary is the closest ancestor node (or this) whose parent doesn't care about the layout of the subtree rooted at the relayout boundary.
There was a problem hiding this comment.
Hmm yeah, I agree. Just pushed a commit that adds an explanation of that; PTAL.
| /// This is set in [layout], and consulted by [markNeedsLayout] in deciding | ||
| /// whether to recursively mark the parent as also needing layout. | ||
| /// | ||
| /// This property is initially null, and becomes null again if this |
There was a problem hiding this comment.
nit: I'm not sure this is an invariant / will be super useful since it seems that relayout boundary is an optimization the render tree does on a best effort basis (markNeedsLayout still has to handle the case where relayout boundary is null). I feel the previous paragraph should suffice?
There was a problem hiding this comment.
Yeah, I think this information about when it's null vs. non-null is less important than the other paragraphs. I initially left it as a TODO comment, and replaced that with this paragraph after the thought was prompted by a subthread above: #150527 (comment)
If you'd prefer to take the paragraph out again, I'd be happy with that.
There was a problem hiding this comment.
Yeah I guess if you want to turn _relayoutBoundary into a boolean flag you'll have to rewrite most of the docs here.
| /// A render object where relayout does not require relayout of the parent | ||
| /// (because its size cannot change on relayout, or because | ||
| /// its parent does not use the child's size for its own layout) | ||
| /// is a "relayout boundary". |
There was a problem hiding this comment.
I believe the description in parentheses here exactly covers the four cases found in layout:
final bool isRelayoutBoundary = !parentUsesSize || sizedByParent || constraints.isTight || parent is! RenderObject;The second and third are two different possible reasons why the size can't change on relayout: the child chooses to base its size only on the constraints, or the constraints dictate a size. The first and last are two different possible ways we can know the parent doesn't use the size: because it said so when calling layout, or because there is no parent.
There was a problem hiding this comment.
(The description also seems clean enough — it makes good logical sense on its own — that I think it's suitable for documentation. The analysis of the actual code is there just for fact-checking, for my own verification that what I'm saying not only sounds logical but is accurate as to the code's actual behavior.)
|
Thanks @LongCatIsLooong for the review! Replied to subthreads above, and pushed a change based on your feedback; PTAL.
Yeah, I think both of these perspectives are helpful. The "what are they for" perspective (special nodes that qualify for certain optimizations) is important for thinking about how they should work and how The "what are key facts that are exactly true about how they currently work" (aka invariants) perspective is one I found important when trying to refactor, and to optimize, how they work (#150524). It helps a lot for making sure I haven't missed something — and for noticing when I have, and need to rework something. I also find it helpful even when trying to fully understand the code that works with this data, because these invariants were often important background assumptions in how the code was written. |
|
(Merged from main to pull in #150707 to fix tests.) |
|
test-exempt: code refactor with no semantic change |
|
Thanks for the reviews! Yeah, definitely one of the nice things about changing it to a boolean is that it becomes simpler to document (because it becomes simpler to understand). In particular the invariant becomes a lot simpler. A major reason I wanted to write down the invariant about null/this/ |
* master: (23 commits) Roll pub packages (flutter#150810) Remove reference to `MaterialApp` and `showCupertinoModalPopup` from `CupertinoAlertDialog` (flutter#150725) Read `AndroidManifest.xml` and emit `manifest-impeller-(enabled|disabled)` analytics (flutter#150791) [flutter_tools] Shut down Chromium cleanly using a command sent through the debug protocol (flutter#150645) Reland fix inputDecorator hint color on M3 (flutter#150278) Roll Flutter Engine from 62e0b5f9c340 to 94023d711db3 (7 revisions) (flutter#150797) Fix collapsed InputDecorator minimum height (flutter#150770) Add more warm up frame docs (flutter#150464) Roll pub packages (flutter#150739) Add `focusNode`, `focusColor`, `onFocusChange`, `autofocus` to `CupertinoButton` (flutter#150721) Document RenderObject._relayoutBoundary and its invariant; small refactors (flutter#150527) Roll Flutter Engine from 6313b1e5afd7 to 62e0b5f9c340 (1 revision) (flutter#150790) fix a typo (flutter#150682) Fix link in RenderObjectWidget doc comment (flutter#150600) Roll Flutter Engine from fbd92055f3a6 to 6313b1e5afd7 (1 revision) (flutter#150781) [tool] make `ErrorHandlingFileSystem.deleteIfExists` catch error code 3 (`ERROR_PATH_NOT_FOUND` on Windows) (flutter#150741) Roll Packages from 711b4ac to 03f5f6d (21 revisions) (flutter#150779) Roll Flutter Engine from afa7ce19bca8 to fbd92055f3a6 (1 revision) (flutter#150777) Reland Add tests for form_text_field.1.dart (flutter#150481) (flutter#150696) (flutter#150750) Add an example for CupertinoPopupSurface (flutter#150357) ...
Manual roll Flutter from e726eb4 to 15f95ce (48 revisions) Manual roll requested by dit@google.com flutter/flutter@e726eb4...15f95ce 2024-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from ddd4814b9d40 to 94591ffb20df (5 revisions) (flutter/flutter#150968) 2024-06-27 34871572+gmackall@users.noreply.github.com Manual engine roll to ddd4814 (flutter/flutter#150952) 2024-06-27 reidbaker@google.com local lint copy gradle task config (flutter/flutter#150957) 2024-06-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from b42c80460538 to d1506c12808e (3 revisions) (flutter/flutter#150951) 2024-06-27 andrewrkolos@gmail.com [tool] make the `systemTempDirectory` getter on `ErrorHandlingFileSystem` wrap the underlying filesystem's temp directory in a`ErrorHandlingDirectory` (flutter/flutter#150876) 2024-06-27 jacksongardner@google.com Have flutter.js load local canvaskit instead of the CDN when appropriate (flutter/flutter#150806) 2024-06-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from a9194f0f01f4 to b42c80460538 (10 revisions) (flutter/flutter#150940) 2024-06-27 jhy03261997@gmail.com [a11y] Reland [#149375 ] Update semantics in dropdown.dart (flutter/flutter#150578) 2024-06-27 goderbauer@google.com Bump dartdoc to 8.0.9+1 (flutter/flutter#150935) 2024-06-27 yjbanov@google.com add onFocus to text fields (flutter/flutter#150648) 2024-06-27 louisehsu@google.com Fixes `flutter build ipa` failure: Command line name "app-store" is deprecated. Use "app-store-connect" (flutter/flutter#150407) 2024-06-27 github@ricardoboss.de Copy any previous `IconThemeData` instead of overwriting it in CupertinoButton (flutter/flutter#149777) 2024-06-27 hans.muller@gmail.com Improve the behavior of scrollbar drag-scrolls triggered by the trackpad (flutter/flutter#150275) 2024-06-27 15272073+Fernthedev@users.noreply.github.com feat: Add autofocus for `MenuItemButton` (flutter/flutter#139396) 2024-06-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d5e3cc55a75 to a9194f0f01f4 (7 revisions) (flutter/flutter#150888) 2024-06-26 34871572+gmackall@users.noreply.github.com Reland "Remove dual_screen from new_gallery integration test" (flutter/flutter#150873) 2024-06-26 parlough@gmail.com Switch to more reliable flutter.dev link destinations in the tool (flutter/flutter#150587) 2024-06-26 goderbauer@google.com Adding `@docImport`s to the `animation` library (flutter/flutter#150798) 2024-06-26 magder@google.com Remove CODEOWNERS trailing whitespace (flutter/flutter#150882) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from e03cf86c4170 to 1d5e3cc55a75 (3 revisions) (flutter/flutter#150875) 2024-06-26 matanlurey@users.noreply.github.com Remind folks we are moving. (flutter/flutter#150872) 2024-06-26 jacksongardner@google.com Remove `bringup: true` from web test shard. (flutter/flutter#150785) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0017bed42c2 to e03cf86c4170 (1 revision) (flutter/flutter#150867) 2024-06-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Remove `dual_screen` from `new_gallery` integration test (#150808)" (flutter/flutter#150871) 2024-06-26 34871572+gmackall@users.noreply.github.com Remove `dual_screen` from `new_gallery` integration test (flutter/flutter#150808) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from d4624a36712b to c0017bed42c2 (4 revisions) (flutter/flutter#150865) 2024-06-26 swrenn@gmail.com Fixes for Style Guide for Flutter Repo (flutter/flutter#150167) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from da62c629ed5c to d4624a36712b (3 revisions) (flutter/flutter#150852) 2024-06-26 sigurdm@google.com Use `Isolate.packageConfigSync! to locate the packageconfig of flutter tools (flutter/flutter#150340) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 25af762ffbb3 to da62c629ed5c (2 revisions) (flutter/flutter#150829) 2024-06-26 polinach@google.com Fix leaky tests. (flutter/flutter#150817) 2024-06-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 94023d711db3 to 25af762ffbb3 (4 revisions) (flutter/flutter#150818) 2024-06-26 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#150810) 2024-06-25 dkwingsmt@users.noreply.github.com Remove reference to `MaterialApp` and `showCupertinoModalPopup` from `CupertinoAlertDialog` (flutter/flutter#150725) 2024-06-25 matanlurey@users.noreply.github.com Read `AndroidManifest.xml` and emit `manifest-impeller-(enabled|disabled)` analytics (flutter/flutter#150791) 2024-06-25 jason-simmons@users.noreply.github.com [flutter_tools] Shut down Chromium cleanly using a command sent through the debug protocol (flutter/flutter#150645) 2024-06-25 bruno.leroux@gmail.com Reland fix inputDecorator hint color on M3 (flutter/flutter#150278) 2024-06-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62e0b5f9c340 to 94023d711db3 (7 revisions) (flutter/flutter#150797) 2024-06-25 bruno.leroux@gmail.com Fix collapsed InputDecorator minimum height (flutter/flutter#150770) 2024-06-25 737941+loic-sharma@users.noreply.github.com Add more warm up frame docs (flutter/flutter#150464) 2024-06-25 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#150739) 2024-06-25 victorsanniay@gmail.com Add `focusNode`, `focusColor`, `onFocusChange`, `autofocus` to `CupertinoButton` (flutter/flutter#150721) 2024-06-25 greg@zulip.com Document RenderObject._relayoutBoundary and its invariant; small refactors (flutter/flutter#150527) 2024-06-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6313b1e5afd7 to 62e0b5f9c340 (1 revision) (flutter/flutter#150790) ...
Fixes part of #150524.
Recently I was studying RenderObject and related code to understand
how the rendering library tracks which objects need layout, and in
particular just how the concept of "relayout boundary" works.
The [RenderObject._relayoutBoundary] property itself had no docs,
and there were some other areas that I felt could be clearer;
so here's a PR where I add those docs.
In addition to docs, this makes some small pure refactors. Mostly
those are in order to make clear docs easier to write. One rename
is to fix a lone straggler from #4425 / #4483.
One key thing I learned, which this documents, is an invariant that
this code maintains on [_relayoutBoundary]. With that invariant in
mind, I found it's possible to simplify that bookkeeping, and to get a
measurable performance improvement as a bonus. To keep things simple,
though, I'll leave those for follow-up PRs.
For ease of review, the PR is broken into several commits each with
their own commit message.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.