-
Notifications
You must be signed in to change notification settings - Fork 46
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
Don't cache subelements #468
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
watt
force-pushed
the
watt/variable-subelements-fix
branch
from
September 20, 2023 18:38
d941d5c
to
f2f03f9
Compare
robmaceachern
approved these changes
Sep 20, 2023
kyleve
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this through!
watt
commented
Sep 21, 2023
Comment on lines
+135
to
+141
return LayoutSubelement( | ||
identifier: identifier, | ||
content: child.content, | ||
environment: environment, | ||
node: node.subnode(key: identifier), | ||
traits: child.traits | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details of the alternatives I attempted:
- Caching each
LayoutSubelement
by identifier. This didn't work becausechild.content
may vary and need to be re-evaluated. - Making
LayoutSubelement
capturechild.content
as a closure and lazily evaluate it once the first time it is needed. Had no impact on performance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In
LayoutStorage
we cache subelements, under the assumption that they will remain stable during a layout pass. This PR removes the caching, because it was a faulty assumption.Details
In
LayoutStorage
we currently assume the array ofLayoutSubelement
s for subelements will be stable during a layout pass, and cache it on theLayoutTreeNode
. This was assumed safe becauseLayoutStorage
-backed themselves elements cannot vary during a layout pass, andLazyStorage
, which can, does not cache its subelement at all.Turns out there is a case where this assumption is violated, if a
LazyStorage
produces a subtree that varies somewhere in a descendant node. For example, aGeometryReader
that contains a stack, and varies the number of children in the stack based on space available.Performance
This PR is broken into 2 commits:
LayoutMode
option. This option was used to performance test this change against the current behavior as a baseline.I was expecting to incur a hit from removing this caching but benchmarking showed it to be completely negligible, and attempts to more cleverly cache actually performed worse. I'll put up a Market PR that shows this benchmark and a regression test.
Testing
There's a new
GeometryReader
test to validate this case, and regression test will be on the Market side.See UI-4599.