Document RenderObject._relayoutBoundary and its invariant; small refactors

[gnprice]

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

• 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.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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

[flutter-dashboard]

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.

[goderbauer]

LGTM. Thanks for the clean-up.

@LongCatIsLooong since you have looked at layout boundaries before, can you also take a look at this?

[goderbauer]

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.

[gnprice]

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.

[goderbauer]

@gnprice you'll also have to get that test exception.

[gnprice]

Thanks for the review! I just bumped my request for that exemption.

[LongCatIsLooong]

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).

[LongCatIsLooong]

How does this reduce closure allocation? The original form doesn't seem to be called in an anonymous function?

[gnprice]

Yeah, the "reduce closure allocation" part isn't relative to before this PR; it was introduced in 1a79592 / #51439.

[LongCatIsLooong]

oh ok looks like the instance method was removed so it makes sense the comment is moved here

[LongCatIsLooong]

nit: cross reference [markNeedsLayout] for what a "relayout boundary" is? I feel the text below doesn't explain that.

[LongCatIsLooong]

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.

[gnprice]

Hmm yeah, I agree. Just pushed a commit that adds an explanation of that; PTAL.

[LongCatIsLooong]

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?

[gnprice]

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.

[LongCatIsLooong]

Yeah I guess if you want to turn _relayoutBoundary into a boolean flag you'll have to rewrite most of the docs here.

[gnprice]

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.

[gnprice]

(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.)

[gnprice]

Thanks @LongCatIsLooong for the review! Replied to subthreads above, and pushed a change based on your feedback; PTAL.

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).

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 markNeedsLayout should handle them. (And I added a paragraph about that in my changes just now.)

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.

[gnprice]

(Merged from main to pull in #150707 to fix tests.)

[stuartmorgan]

test-exempt: code refactor with no semantic change

[LongCatIsLooong]

LGTM. I think most of the comments you added here will be removed when you change _relayoutBoundary to a boolean (and I think it would be easier to document if it was a boolean).

[gnprice]

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/parent!._relayoutBoundary! is really just that it sets up for that refactor that turns _relayoutBoundary into a boolean — I found that that was the key information that helped me reason through verifying each step of the refactor. When I send that next PR, after this lands and I rebase atop it, you'll see that in the commit messages.