Started sharing SemanticsProperties between the Widget and the RenderObject #104281

gaaclarke · 2022-05-20T18:47:55Z

This starts sharing memory between Semantics and RenderSemanticsAnnotations. I noticed a significant decrease in build times in customer: money when I ripped out all the Semantics while testing something else. I theorized that the performance increase was related to the large memory cost of Semantics where almost all of its 62 instance variables were getting copied to the RenderObject. In production apps Semantics widgets become ubiquitous and account for a large percentage of the allocations.

I did a bit of testing to see what effect his might have. I ran a before and after with customer: money where I just clicked on the first button to load the main screen and saw 108MB of allocations go to 85MB, where 210 RenderSemanticsAnnotations where allocated. That would mean a savings of ~112 KB per RenderSemanticsAnnotations. That doesn't sound plausible, I'm not sure what Dart is doing under the covers though. I would have expected something more like (48 removed ivars * ~16 bytes * 210 instances) = 161 KB total.

I did run CPU benchmark tests with build_bench.dart and didn't notice a difference, but it also isn't that representative. I did try my customer: money benchmark and didn't notice a difference either but that benchmark isn't quite good either.

I don't think it's worth spending more time trying to track down the exact extent of the changes. The cleanup of this optimization is value enough. Writing a microbenchmark just for this may not be worth it. Hopefully I can get the customer:money one showing what we want eventually.

before:

after:

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.
All existing and new tests are passing.

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

…Object.

packages/flutter/test/rendering/reattach_test.dart

Hixie · 2022-05-20T23:16:40Z

test-exempt: code refactor with no semantic change

however if this breaks anything in flutter/tests or google, we will need a migration guide
it might be worth pro-actively searching for potentially broken code in Google that might not be covered by tests.

Hixie

cc @goderbauer for visibility

Co-authored-by: Ian Hickson <[email protected]>

gaaclarke · 2022-05-20T23:38:51Z

it might be worth pro-actively searching for potentially broken code in Google that might not be covered by tests.

I saw no references of subclassing RenderSemanticsAnnotations or allocating it. I think those are things that would potentially break.

goderbauer · 2022-05-20T23:50:05Z

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


+  SemanticsProperties _properties;
+  /// Sets the [SemanticsProperties] en masse.
+  void updateProperties(SemanticsProperties value) {


Why is this not just a regular setter for properties?

Because it's a linter error to have a setter without a getter. I guess I could add a getter safely since it's effectively immutable. I tried to give out the least amount of access necessary.

Yeah, having a getter for this is normal style for RenderObjects. Adding one should be fine.

goderbauer · 2022-05-20T23:52:32Z

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

-    bool? liveRegion,
-    int? maxValueLength,
-    int? currentValueLength,
    AttributedString? attributedLabel,


These also exist on SmeanticsProperties, no? Why are those allowed to be passed in separately?

They had slightly different logic where they get wrapped in AttributeString. I was afraid that if I moved it, I might accidentally start allocating more than one AttributeString. It was hard for me to prove that I wouldn't without storing that value somewhere.

It would be fine to store that value on the RenderObject. It's just confusing that the API of the RO allowed this value to be passed in in two different ways and one just silently gets ignored.

Done, moved all that logic from the widget into the renderobject.

goderbauer · 2022-05-20T23:54:09Z

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

-    _checked = value;
-    markNeedsSemanticsUpdate();
-  }
+  bool? get checked => _properties.checked;


I would be in favor of removing these getters as well, if that's not breaking anything. Are these actually used anywhere?

They are only used locally in the framework afaik. They are probably used in tests. There is tons of documentation written for them. I was worried that some of that information might only live there. It seems like SemanticsProperties is the proper place for that documentation though.

+1. If this information is not also available on SemanticsProperties let's move it there and get rid of these here.

Okay, I skimmed through the docstrings and they seem like copy and paste. I don't think there is anything lost ripping them out and all the tests passed locally so I think we are good. Done.

goderbauer · 2022-05-20T23:56:08Z

packages/flutter/lib/src/widgets/basic.dart

-      hidden: properties.hidden,
-      image: properties.image,
+      properties: properties,
      attributedLabel: _effectiveAttributedLabel,


Ah! Maybe we should move the logic for figuring the effectiveAttributed* also down one level to avoid the confusion that attributedLabel is passed in directly, but also as part of the properties.

goderbauer · 2022-05-20T23:57:55Z

packages/flutter/lib/src/widgets/basic.dart

-      ..onDidGainAccessibilityFocus = properties.onDidGainAccessibilityFocus
-      ..onDidLoseAccessibilityFocus = properties.onDidLoseAccessibilityFocus
-      ..customSemanticsActions = properties.customSemanticsActions;
+      ..textDirection = _getTextDirection(context);


(unrelated) I wonder if we have a "bug" here: properties also sets a textdirection, but we seem to just ignore that one...

Yea, it seemed weird, but I just tried not to touch it. _getTextDirection does use the value of properties if it is available fwiw.

Ah, cool. That makes sense then.

goderbauer · 2022-05-21T00:01:08Z

Nice clean-up overall!

dnfield · 2022-05-21T02:42:41Z

test-exempt: code refactor with no semantic change

Uhhh isn't this entirely a semantic change?

I'll leave now...

…to share-sematics-properties

…nterface

goderbauer

LGTM

goderbauer · 2022-05-23T18:09:18Z

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

+
+  SemanticsProperties _properties;
+  /// Sets the [SemanticsProperties] en masse.
+  void updateProperties(SemanticsProperties value) {


Make this a regular getter/setter pair to follow RenderObject convention? Since SemanticsProperties is immutable anyways, this shouldn't cause any trouble.

goderbauer · 2022-05-23T18:14:37Z

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

+    if (_properties == value)
+      return;
+    _properties = value;
+    _updateAttributedFields(_properties);


In theory, if we are worried about unnecessary object instantiations, we could just reset the attributed fields to null here and do this pre-computation work the first time describeSemanticsConfiguration is called. That way, the AttributedStrings would actually only get instantiated when they are needed (i.e. when Semantics are enabled). Not sure how much this matters. Leaving this as-is is also fine.

I did play around with making it late but I was worried about getting stale data on accident. I just kept it the same.

…e RenderObject (flutter/flutter#104281)

…Object (flutter#104281)

…e RenderObject (flutter/flutter#104281)

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 20, 2022

gaaclarke changed the title ~~Started sharing SemanticsProperties between the Widget and the Render…~~ Started sharing SemanticsProperties between the Widget and the RenderObject May 20, 2022

gaaclarke force-pushed the share-sematics-properties branch 3 times, most recently from b4a59d1 to e873d86 Compare May 20, 2022 20:47

flutter-dashboard bot added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label May 20, 2022

Started sharing SemanticsProperties between the Widget and the Render…

f55eb80

…Object.

gaaclarke force-pushed the share-sematics-properties branch from e873d86 to f55eb80 Compare May 20, 2022 21:52

gaaclarke marked this pull request as ready for review May 20, 2022 23:02

gaaclarke requested a review from goderbauer May 20, 2022 23:07

Hixie reviewed May 20, 2022

View reviewed changes

packages/flutter/test/rendering/reattach_test.dart Outdated Show resolved Hide resolved

Hixie approved these changes May 20, 2022

View reviewed changes

Update packages/flutter/test/rendering/reattach_test.dart

bed80f8

Co-authored-by: Ian Hickson <[email protected]>

goderbauer reviewed May 20, 2022

View reviewed changes

gaaclarke added 3 commits May 23, 2022 10:25

removed old getters

93a033f

Merge remote-tracking branch 'gaaclarke/share-sematics-properties' in…

628cb6a

…to share-sematics-properties

moved attributed variables out of RenderSemanticsAnnotations public i…

2f312f7

…nterface

gaaclarke requested a review from goderbauer May 23, 2022 18:06

goderbauer approved these changes May 23, 2022

View reviewed changes

gaaclarke added 2 commits May 23, 2022 14:15

fixed docstrings, made properties a property

ab6763b

updated docstring

09ec7eb

gaaclarke added the waiting for tree to go green label May 23, 2022

fluttergithubbot merged commit 914556e into flutter:master May 23, 2022

engine-flutter-autoroll mentioned this pull request May 23, 2022

Roll Flutter from 7ece8f9f9435 to a1bce9d0097c (3 revisions) flutter/packages#2045

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 23, 2022

914556e Started sharing SemanticsProperties between the Widget and th…

c73b87f

…e RenderObject (flutter/flutter#104281)

engine-flutter-autoroll mentioned this pull request May 24, 2022

Roll Flutter from 7ece8f9f9435 to a1bce9d0097c (3 revisions) flutter/plugins#5814

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 24, 2022

914556e Started sharing SemanticsProperties between the Widget and th…

8564a31

…e RenderObject (flutter/flutter#104281)

engine-flutter-autoroll mentioned this pull request May 24, 2022

Roll Flutter from 7ece8f9f9435 to f85209272dca (5 revisions) flutter/plugins#5815

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 24, 2022

914556e Started sharing SemanticsProperties between the Widget and th…

2c71fc0

…e RenderObject (flutter/flutter#104281)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Started sharing SemanticsProperties between the Widget and the Render…

710fc5d

…Object (flutter#104281)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

914556e Started sharing SemanticsProperties between the Widget and th…

23f23db

…e RenderObject (flutter/flutter#104281)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

914556e Started sharing SemanticsProperties between the Widget and th…

39b877d

…e RenderObject (flutter/flutter#104281)

Started sharing SemanticsProperties between the Widget and the RenderObject #104281

Started sharing SemanticsProperties between the Widget and the RenderObject #104281

Uh oh!

Conversation

gaaclarke commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Hixie commented May 20, 2022

Uh oh!

Hixie left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

goderbauer commented May 21, 2022

Uh oh!

dnfield commented May 21, 2022

Uh oh!

goderbauer 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaaclarke commented May 20, 2022 •

edited

Loading

gaaclarke commented May 20, 2022 •

edited

Loading