Introduce caching mechanism during compile semantics tree #150394

chunhtai · 2024-06-17T22:48:20Z

There are two expansive part when updating a semantics subtree.

_getSemanticsForParent to get the _SemanticsFragment
_SemanticsFragment.compileChildren to get the actual semantics node.

This pr introduced caching for both steps.

First, this pr make a _RenderObjectSemantics to replace _SemanticsFragment. It will be permanently cached on each RenderObject. It handles any semantics related compilation and move all the properties from RenderObject to this class. RenderObject is only responsible for provide semantics property through describeSemanticsConfiguration and assembleSemanticsNode.

to compile the semantics sub-tree,

the root of the RenderOjbect of that subtree will first gather all the other _RenderObjectSemantics in the subtree that contribute to semantics. (cached as mergeUp, and siblingMergeGroups)
It handles merging and filter out those that won't form a semantics node to form a _RenderObjectSemantics tree that each node in the tree will form semantics nodes. (cached as _childrenAndElevationAdjustments)
go through the _RenderObjectSemantics tree to create semantics tree. (cached as semanticsNodes)

The performance boost comes from caching from all steps. When the markNeedsSemanticsUpdate is called. it will only clear minimum caches from the _RenderObjectSemantics so only a small portion will be recalculated.

before

after

getsemantics + compileChildren
530 + 470 ~= 900 before
34 + 187 ~= 200 after

about 78% reduce in compile time

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 · 2024-06-17T22:48:24Z

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.

OverlayPortal attaches its overlaychild's renderobject to overlay directly while keeps its semantics tree under overlayportal. This become a problem when the `overlaychild` markNeedsSemanticsUpdate that it propagate upward the rendering tree. This means instead of marking dirty upward to the OverlayPortal, it directly mark Overlay dirty instead and skip `OverlayPortal`. Currently this does not pose an issue other than unnecessary rebuilds, but it become a problem when I try to optimize the semantics tree compilation #150394. After the optimization it won't rebuild semantics node unless it is marked dirty. Since the OverlayPortal widget does not get marked dirty, it won't update the subtree.

chunhtai · 2024-07-31T22:45:33Z

latest
before
877us + 807us in 3118us frame

after
57us + 273us in 2200us frame

chunhtai · 2024-08-05T22:31:19Z

some thoughts after working on reducing semantics compiling time. There are so many corner cases that ain't previously present since we used to recompile entire subtree, and each of them is very hard to debug because the code is too convoluted.

There are somethings I noticed that are very hard to make changes to this part of the code.

parent inherited property like mergeintoparent and blocksUserActions and geometry is very hard to update when it changes. It not only has to update its own semantics fragment, it also has to update caches such as semanticsConfiguration and SemanticsNode it creates. It also needs to conditional update the entire subtree. It feels like I have to write one off logic for each properties.
fragment creates a mutable version of SemanticsConfiguration and uses it to generate semantics node. Previously, fragment is recreated every time and this is mostly fine. but now we caches fragment. This makes things hard when updating fragment, it needs to figure out when and whether to update its mutable SemanticsConfiguration, and sometimes i need to reset it in a tricky way so a parent fragment can still make sense of a child fragment's semantics configuration. Also sometimes it has to look up original semantics configuration instead of the mutable copy. This creates a bunch of corner cases that I have to write a one off logic for it.
SemanticsNode.isHidden and whether the semantics node should be dropped from the tree is built with assumption that the entire semantics node subtree will be recreated. It relies on the order for _getSemanticsForParent and compileChildren and how SemanticsNode._replaceChildren to correctly set each flag during progress to get it right.

for 1, The caches should probably have a more unify API for updating child fragments, and we can also think about changing how implicit merging work. This will be a big breaking change though.

For 2, I think in the future we should probably do something to keep a single source of true and avoid mutating SemanticsConfiguration. Though this ties to 1 where some modification to SemanticsConfiguration is due to implicit merge.

Fixing all these problem and adding caches seems too dramatic for a single change. so I decided to leave these problem for a later time.

…#151688) OverlayPortal attaches its overlaychild's renderobject to overlay directly while keeps its semantics tree under overlayportal. This become a problem when the `overlaychild` markNeedsSemanticsUpdate that it propagate upward the rendering tree. This means instead of marking dirty upward to the OverlayPortal, it directly mark Overlay dirty instead and skip `OverlayPortal`. Currently this does not pose an issue other than unnecessary rebuilds, but it become a problem when I try to optimize the semantics tree compilation flutter#150394. After the optimization it won't rebuild semantics node unless it is marked dirty. Since the OverlayPortal widget does not get marked dirty, it won't update the subtree.

flutter-dashboard · 2024-10-28T22:41:54Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

mdebbar

I'm still going through the PR. Here's my first set of comments.

mdebbar · 2024-10-31T15:57:54Z

packages/flutter/lib/src/rendering/object.dart

+      // first, it may use dirty geometry in parent's semantics node to
+      // calculate the geometries in the subtree.
+      final List<RenderObject> nodesToProcess = _nodesNeedingSemantics
+        .where((RenderObject object) => !object._needsLayout && object.owner == this)


From the dartdocs above, the flushSemantics method is called after painting. Is it possible for a render object to need layout right after painting?

that is not possible, the flushpaint will ensure the layout is not dirty at the end

Follow up question: why are we filtering out objects that need layout?

Isn't this the same as:

Suggested change

.where((RenderObject object) => !object._needsLayout && object.owner == this)

.where((RenderObject object) => object.owner == this)

IIrc widget that are offstage won't layout during the layout stage, those renderobject may still end up here if they markNeedsSemanticsUpdate. We skip those since they won't participate in semantics tree anyway.

packages/flutter/lib/src/rendering/object.dart

mdebbar · 2024-11-01T19:23:00Z

packages/flutter/lib/src/rendering/object.dart

+      });
+    }
+
+    final bool effectiveBlocksUserAction = newParentData.blocksUserActions || configProvider.original.isBlockingUserActions;


Related to the original vs effective comment:

Could you clarify why we are getting the effective "block user action" from original but then comparing it with the one in effective?

My understanding so far is that effective is either identical to original OR a writable copy of original. And by doing setProperty below, you are basically making sure that effective is a writable copy of original and then you update it.

If my understanding is correct, then I think it makes more sense to do:

Suggested change

final bool effectiveBlocksUserAction = newParentData.blocksUserActions || configProvider.original.isBlockingUserActions;

final bool effectiveBlocksUserAction = newParentData.blocksUserActions || configProvider.effective.isBlockingUserActions;

the effective may contain the stale data from previous update.

for example
frame1: parent blocks user actions = true, this configProvider.effective.isBlockingUserActions will be set to true

frame 2: parent blocks user actions updates to false, the configProvider.effective.isBlockingUserActions suppose to be updated to false, but effective still contains stale data so we have to check the original config instead

This is one of the reasons why I have to keep an original and effective config

Thanks for clarifying.

Based on your description of effective and my understanding of this PR, I suggest the following changes to make it slightly more readable (IMHO):

SemanticsConfiguration get original { if (_originalCache == null) { // No need to set two variables here. Just one is enough. _originalCache = SemanticsConfiguration(); // ..... } return _originalCache; } SemanticsConfiguration? _originalCache; // This is only used in this file, no need to make it public. // // In fact, since this is a mutable copy of the config, I strongly prefer // that it remains private. SemanticsConfiguration? _mutableConfig; void updateConfig(callback) { if (_mutableConfig == null) { _mutableConfig = original.copy(); } callback(_mutableConfig); }

This makes it a bit clearer from my perspective, but I'm also curious what the other reviewers think about this?

Also, given that we are using .original and .effective in different parts of the code, it would be great if you could write a little comment explaining when one should be used vs the other.

For example, in ensureSemanticsNode() and some getters above, we are using .effective.isSemanticBoundary etc. Why not .original.isSemanticBoundary?

The entire class is private so I dont think it really matter, but I try to maintain certain code style that property used outside of this class is public while internal property stay private

also I think mutable doesn't convey the meaning well, it state the what happen to it but is not helpful that when it should be used.

chunhtai · 2024-11-04T20:32:14Z

This pr is ready for review

mdebbar · 2024-11-07T17:47:58Z

packages/flutter/lib/src/rendering/object.dart

+      // first, it may use dirty geometry in parent's semantics node to
+      // calculate the geometries in the subtree.
+      final List<RenderObject> nodesToProcess = _nodesNeedingSemantics
+        .where((RenderObject object) => !object._needsLayout && object.owner == this)


Follow up question: why are we filtering out objects that need layout?

Isn't this the same as:

Suggested change

.where((RenderObject object) => !object._needsLayout && object.owner == this)

.where((RenderObject object) => object.owner == this)

mdebbar · 2024-11-12T18:41:46Z

packages/flutter/lib/src/rendering/object.dart

+      });
+    }
+
+    final bool effectiveBlocksUserAction = newParentData.blocksUserActions || configProvider.original.isBlockingUserActions;


Thanks for clarifying.

Based on your description of effective and my understanding of this PR, I suggest the following changes to make it slightly more readable (IMHO):

SemanticsConfiguration get original { if (_originalCache == null) { // No need to set two variables here. Just one is enough. _originalCache = SemanticsConfiguration(); // ..... } return _originalCache; } SemanticsConfiguration? _originalCache; // This is only used in this file, no need to make it public. // // In fact, since this is a mutable copy of the config, I strongly prefer // that it remains private. SemanticsConfiguration? _mutableConfig; void updateConfig(callback) { if (_mutableConfig == null) { _mutableConfig = original.copy(); } callback(_mutableConfig); }

This makes it a bit clearer from my perspective, but I'm also curious what the other reviewers think about this?

mdebbar · 2024-11-12T18:48:00Z

packages/flutter/lib/src/rendering/object.dart

+      });
+    }
+
+    final bool effectiveBlocksUserAction = newParentData.blocksUserActions || configProvider.original.isBlockingUserActions;


Also, given that we are using .original and .effective in different parts of the code, it would be great if you could write a little comment explaining when one should be used vs the other.

For example, in ensureSemanticsNode() and some getters above, we are using .effective.isSemanticBoundary etc. Why not .original.isSemanticBoundary?

packages/flutter/lib/src/rendering/object.dart

mdebbar · 2024-11-15T21:28:18Z

packages/flutter/lib/src/rendering/object.dart

+        );
+      }
+    }
+    mergeUp.addAll(children);


It feels backwards (and probably extra work) to add items to the mergeUp list and later do:

mergeUp.clear(); mergeUp.add(this);

Why not something like this:

// mergeUp.addAll(children); // <-- remove this line if (contributesToSemanticsTree) { // ... _marksExplicitInMergeGroup(children, isMergeUp: true); // ... mergeUp.add(this); } else { mergeUp.addAll(children); }

mergeUp is used in the if case to get the mergeUpConfigs.

packages/flutter/lib/src/rendering/object.dart

chunhtai · 2025-01-06T23:42:43Z

closing for #161195

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 17, 2024

chunhtai force-pushed the issues/150234 branch from dcd1341 to e9d4df2 Compare July 1, 2024 21:59

github-actions bot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Jul 1, 2024

chunhtai force-pushed the issues/150234 branch 3 times, most recently from 7da6e2b to 7936617 Compare July 8, 2024 22:00

chunhtai mentioned this pull request Jul 12, 2024

Updates mark needs semantics update logic for overlay portal #151688

Merged

9 tasks

github-actions bot added the f: scrolling Viewports, list views, slivers, etc. label Jul 16, 2024

chunhtai force-pushed the issues/150234 branch from 1fdb37b to 1ed87e9 Compare July 16, 2024 01:49

github-actions bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. and removed a: text input Entering text in a text field or keyboard related problems labels Jul 18, 2024

chunhtai marked this pull request as draft July 24, 2024 18:31

chunhtai force-pushed the issues/150234 branch 2 times, most recently from b606224 to fe69ce8 Compare July 26, 2024 21:01

chunhtai force-pushed the issues/150234 branch from 6449745 to 8873cd5 Compare July 31, 2024 22:17

github-actions bot removed f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. labels Jul 31, 2024

chunhtai requested a review from goderbauer August 1, 2024 18:17

chunhtai force-pushed the issues/150234 branch from ab56e70 to fd5ab3a Compare August 6, 2024 22:35

chunhtai force-pushed the issues/150234 branch 3 times, most recently from 54b6333 to bf0a7d8 Compare August 12, 2024 22:26

chunhtai marked this pull request as ready for review August 13, 2024 20:50

chunhtai force-pushed the issues/150234 branch from 791e2a7 to 93fa36e Compare August 22, 2024 23:13

chunhtai force-pushed the issues/150234 branch from b237acf to d3fb5c1 Compare September 20, 2024 21:19

chunhtai force-pushed the issues/150234 branch 3 times, most recently from 6e2e0dc to 08a63d6 Compare October 8, 2024 20:07

chunhtai force-pushed the issues/150234 branch 2 times, most recently from accdc52 to b29f972 Compare October 30, 2024 18:12

mdebbar reviewed Nov 1, 2024

View reviewed changes

chunhtai force-pushed the issues/150234 branch from 23139cc to b1c8fe2 Compare November 4, 2024 18:59

chunhtai requested review from goderbauer and mdebbar November 4, 2024 20:31

mdebbar reviewed Nov 15, 2024

View reviewed changes

chunhtai force-pushed the issues/150234 branch from b63c4e0 to bb89af2 Compare November 20, 2024 22:48

chunhtai requested a review from mdebbar November 20, 2024 22:56

chunhtai added 8 commits November 22, 2024 12:01

Introduces caching to semantics compilation

5145c71

update

bee78b8

update

ea4a1a0

update

9440fa9

update

acd802c

update

2f50fc1

update

e442fab

update

5e9e921

chunhtai force-pushed the issues/150234 branch from b172af0 to 5e9e921 Compare November 22, 2024 20:03

update

989ac5a

chunhtai mentioned this pull request Jan 6, 2025

Introduce caching mechanism during compile semantics tree monorepo and formatted version #161195

Merged

9 tasks

chunhtai closed this Jan 6, 2025

	.where((RenderObject object) => !object._needsLayout && object.owner == this)
	.where((RenderObject object) => object.owner == this)

	final bool effectiveBlocksUserAction = newParentData.blocksUserActions \|\| configProvider.original.isBlockingUserActions;
	final bool effectiveBlocksUserAction = newParentData.blocksUserActions \|\| configProvider.effective.isBlockingUserActions;

Introduce caching mechanism during compile semantics tree #150394

Introduce caching mechanism during compile semantics tree #150394

Uh oh!

Conversation

chunhtai commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jun 17, 2024

Uh oh!

chunhtai commented Jul 31, 2024

Uh oh!

chunhtai commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Oct 28, 2024

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chunhtai commented Jun 17, 2024 •

edited

Loading

chunhtai commented Aug 5, 2024 •

edited

Loading