Reduce relayout-boundary tracking to a bool-or-null per RenderObject #150905
Conversation
In the later commits of this PR, we'll step by step convert all the references to _relayoutBoundary to use _isRelayoutBoundary instead, and then eliminate _relayoutBoundary in favor of _isRelayoutBoundary. With that in mind, we also go ahead and move most of the documentation from _relayoutBoundary to the new _isRelayoutBoundary.
…ough Each of these changes can be verified as a pure refactor just from the definition of the _isRelayoutBoundary getter. (At one spot, in _debugRelayoutBoundaryAlreadyMarkedNeedsLayout, one also needs the `node != _relayoutBoundary` condition from the line just above the change.) Other references to _relayoutBoundary, where the conversion requires a bit more subtlety to reason through, are broken out into subsequent commits.
This is a pure refactor, because markParentNeedsLayout was already setting _needsLayout to true in the one case where this function wasn't. A similar simplification was already possible when this was in terms of _relayoutBoundary rather than _isRelayoutBoundary, but was somewhat harder to see.
This commit converts two uses of _relayoutBoundary where its value is being compared against an ancestor, in the course of walking up the ancestry chain. Each of these changes can be verified as a pure refactor by using the ancestry invariant documented on _relayoutBoundary.
This relaxes the conditions under which [layout] causes a walk of the tree to update or clear relayout boundaries. Previously we did so when _relayoutBoundary would change; now we do so only when its tristate counterpart _isRelayoutBoundary would change. This is a pure refactor because, in fact, the old and new conditions are equivalent: the only times when _relayoutBoundary would change are times when _isRelayoutBoundary would also change (and vice versa). That's true because of the invariant documented on _relayoutBoundary, namely that that property's value is either null, `this`, or `parent!.relayoutBoundary!`. The relayoutBoundary local, from its definition, is also either `this` or `parent!.relayoutBoundary!`. As a result, the values relayoutBoundary and _relayoutBoundary will differ only when isRelayoutBoundary and _isRelayoutBoundary also differ.
At this stage, the only places that ever read _relayoutBoundary are either in the _isRelayoutBoundary getter, or purely in order to copy the value to another object's _relayoutBoundary. That means the only information we ever ultimately use from _relayoutBoundary is the information that's contained in the simpler _isRelayoutBoundary. As a result, we can make _isRelayoutBoundary a field to store that information directly, and cut out _relayoutBoundary entirely.
|
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. |
github-actions
bot
added
the
framework
|
This doesn't yet introduce the changes that I expect to have any effect on performance — those are coming next after this PR. I don't observe any effect, either, when I run the microbenchmarks in #150539. So even if we do want to land some version of those new benchmarks before merging the changes that have performance effects, I think this PR can go ahead independently of the benchmarks PR. |
| /// is one of `null`, `this`, or `parent!._relayoutBoundary!`.) | ||
| RenderObject? _relayoutBoundary; | ||
| /// When [_isRelayoutBoundary] is false, then `parent?._isRelayoutBoundary` | ||
| /// may be true or false but not null. As a result, when it's known that |
There was a problem hiding this comment.
nit: remove double space after .
There was a problem hiding this comment.
Also: The last sentence in this paragraph reads somewhat awkward. Maybe that can be rephrased?
| assert(node.parent != null); | ||
| node = node.parent!; | ||
| if ((!node._needsLayout) && (!node._debugDoingThisLayout)) { | ||
| return false; | ||
| } | ||
| } | ||
| assert(node._relayoutBoundary == node); | ||
| assert(node._isRelayoutBoundary ?? false); |
There was a problem hiding this comment.
For readibility and understandability, I'd find it clearer to express this assert as
| assert(node._isRelayoutBoundary ?? false); | |
| assert(node._isRelayoutBoundary == true); |
to indicate that that node absolutely must be a relayout boundary.
There was a problem hiding this comment.
Oh, the use_if_null_to_convert_nulls_to_bools lint probably forced this? Never mind then.
| // _relayoutBoundary is cleaned by an ancestor in RenderObject.layout. | ||
| // Conservatively mark everything dirty until it reaches the closest | ||
| // known relayout boundary. |
There was a problem hiding this comment.
Isn't this comment still valid for parts of the branch (i.e. when _isRelayoutBoundary == null)?
| child.visitChildren(_cleanChildRelayoutBoundary); | ||
| child._relayoutBoundary = null; | ||
| child._isRelayoutBoundary = null; | ||
| } | ||
| } | ||
|
|
||
| // This is a static method to reduce closure allocation with visitChildren. | ||
| static void _propagateRelayoutBoundaryToChild(RenderObject child) { |
There was a problem hiding this comment.
maybe rename this to indicate that it will mark the child to NOT be a relayout boundary.
| /// Set [_isRelayoutBoundary] to [value] on this render object, | ||
| /// then to false throughout the rest of its subtree, | ||
| /// stopping at relayout boundaries. | ||
| void _setIsRelayoutBoundary(bool value) { |
There was a problem hiding this comment.
The name of this method makes it seem as if you can replace it with _isRelayoutBoundary = value. Maybe indicate in the name that this includes recursing down the tree?
There was a problem hiding this comment.
Also, some of this walking is now unnecessary as you've indicated in your comment, right? Maybe leave a TODO somewhere here that you can remove in your next PR to avoid confusion for anyone reading this code before that PR lands.
There was a problem hiding this comment.
Actually, I am wondering why we want to do this in two separate PRs instead of including it all in this one? If the next step for whatever reason doesn't work out leaving the code in this state is kinda awkward.
| int count = 1; | ||
| RenderObject? target = parent; | ||
| while (target != null && target != _relayoutBoundary) { | ||
| while (target != null && target._isRelayoutBoundary == false) { |
There was a problem hiding this comment.
Shouldn't this be target._isRelayoutBoundary != true?
| @@ -2249,20 +2248,20 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge | |||
| static bool debugCheckingIntrinsics = false; | |||
|
|
|||
| bool _debugRelayoutBoundaryAlreadyMarkedNeedsLayout() { | |||
| if (_relayoutBoundary == null) { | |||
| if (_isRelayoutBoundary == null) { | |||
There was a problem hiding this comment.
Not related to this PR, but once you change _isRelayoutBoundary from bool? to bool, is it still possible to keep this assert? Since here it actually cares whether it is null, instead of treating null as false like everywhere else?
|
Hi @gnprice, are you still planning on coming back to this pr? |
|
Yeah, I'm still planning to return to this. The summer has been a busy time, but I'm hoping to pick it back up in September or October. |
|
Hey @gnprice, greetings from PR triage, is this still on your radar? |
|
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to re-open or submit a new PR if you have the time to address the review comments. Thanks! |
|
@gnprice any chance this is going to be brought back? It seems to be a good improvement! |
Fixes part of #150524.
As documented on
_relayoutBoundary(since e5a35f4 / #150527),that property has three possible states: it can be null,
this,or some ancestor; and in the case of an ancestor, the question of
which ancestor is answered by the same property on the parent.
This makes for a lot of redundancy between the
_relayoutBoundaryvalues on different render objects in the tree. All the same
information can be expressed by a
bool?property on each objectexpressing whether the object is itself a relayout boundary;
call it
_isRelayoutBoundary.The existing property of type
RenderObject?does provide a directlookup of which ancestor, specifically, is the relayout boundary.
But it turns out that (a) that extra information is never used at all
in release builds; and (b) even in debug builds, it's only used in
conjunction with walking the parent chain up to that ancestor, at
which point the
bool?values give us all the same information.This is a pure refactor; all the outward behavior remains the same.
The main immediate win is that it makes the logic simpler, and makes
it simpler to understand by having each render object keep track of
only the data that the logic on that render object will actually use.
As a bonus, this unlocks a small but measurable performance
improvement by letting us reduce the bookkeeping we do on these
values. That part isn't quite a pure refactor, though, so I'll
leave it for a follow-up PR.
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.