Made Directionality forego dependency tracking for better performance. by gaaclarke · Pull Request #102336 · flutter/flutter

gaaclarke · 2022-04-21T21:34:08Z

Customer money is spending 20ms of startup time doing bookkeeping for Directionality This gets rid of that bookkeeping which makes updates faster, but Directionality should be changed infrequently. On some platforms change in text direction actually forces reloading the process anyways (iOS).

Local testing of this change to the increase stock_build_iteration 2.5% (21ms).

I tried to make this solution more general but there was apprehension to change the public API and it seemed like other Widgets didn't have the same draw as Directionality.

Examples of widgets using Directionality.of:

Semantics.getTextDirection
createLocalImageConfiguration
Transform.updateRenderObject
FittedBox.createRenderObject

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.

flutter-dashboard · 2022-04-21T21:34:11Z

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

gaaclarke · 2022-04-22T18:00:47Z

The widget test is legitimately broken. I'm going to take this PR out of review until I fixed it.

gaaclarke · 2022-04-22T22:15:31Z

Okay, I fixed the tests. I previously wasn't recursing into the children elements and I had to make updates get filtered based on the bookkeeping we are keeping lower in the tree to match existing behavior. I had to make one tiny addition to the Element api.

jonahwilliams · 2022-04-25T17:09:55Z

Some questions I have about this approach - how well does this scale in the odd case where users override directionality? What is the cost of updating the widget tree if the directionality does change?

Finally, how does the performance improvement scale to less constrained devices? I'm assuming the number comes from a wembly?

goderbauer · 2022-04-25T17:26:23Z

Local testing of this change to the increase stock_build_iteration 2.5% (21ms).

What does this mean? Did it make the benchmark worse?

What is the expected performance benefit of this change?

goderbauer · 2022-04-25T17:24:22Z

could just be an assert?

I thought about it but I don't know what our policy is there. You probably know better. We can't guarantee that the assert would always get hit in debug build and not release build. If Directionality ends up being the only user of it it might not be a big deal. I was just erring on the side of caution.

I would just go with the assert here for now.

(oops, fixed it, good now)

goderbauer · 2022-04-25T17:30:45Z

nit: a future reader will probably appreciate a little more documentation on how this is different from the regular InheritedElement.

goderbauer · 2022-04-25T17:32:18Z

This override seems unnecessary?

I did this in order to convert a @protected method to a public one. I don't know if there is an easier way.

Oh, right. Makes sense then :)

goderbauer · 2022-04-25T17:36:14Z

For my own understanding: Is the optimization here that we omit the bookkeeping that originally happened in this method? (Adding the dependent to a map?)

Yep, that's correct. You can see that in the profile I linked in the internal bug.

Maybe leave that as a comment here for future readers, who want to understand why we did this.

gaaclarke · 2022-04-25T17:37:55Z

Local testing of this change to the increase stock_build_iteration 2.5% (21ms).

What does this mean? Did it make the benchmark worse?

Nope, an increase in performance, a decrease in time. I wouldn't recommend a change that doesn't help haha.

What is the expected performance benefit of this change?

In the stock_build_iteration test, run locally on my iPhone SE, the build times have improved 2.5% (21ms). I think that benchmark is pretty representative of many of our users UIs. I have profiles of the startup time of customer: money on low end Android devices which show it taking up 20ms of startup time in this bookkeeping as well. Details can be found on b/229707977.

goderbauer · 2022-04-25T17:40:01Z

I have profiles of the startup time of customer: money on low end Android devices which show it taking up 20ms of startup time in this bookkeeping as well. Details can be found on b/229707977.

Does this change translate to a 20ms faster startup time for customer:money then?

jonahwilliams · 2022-04-25T17:41:21Z

Ahh Iphone SE, not bad

jonahwilliams · 2022-04-25T17:41:56Z

Another follow up question: can we identify other inherited widgets that frequently don't change?

gaaclarke · 2022-04-25T17:46:06Z

Some questions I have about this approach - how well does this scale in the odd case where users override directionality? What is the cost of updating the widget tree if the directionality does change?

We are switching our algorithm from:

insertion: average O(1) to O(1), worst case O(n) to O(1) (edit: well even better than O(1) since nothing is happening, is O(0) a thing? haha)
lookup: average O(1) to O(n), worst case O(n) to O(n)

Insertions are happening when we are building the widget tree, lookups happen when the widget decides its data has changed and needs to notify its dependencies.

So, the performance will be somewhere between where it was before the change and a hot reload. It won't be as costly as a hot reload because we are still filtering updates to widgets that actually depend on the widget. I think in practice Directionality is so ubiquitous that the delta won't be that much different.

Finally, how does the performance improvement scale to less constrained devices? I'm assuming the number comes from a wembly?

My local testing was done on a modern iPhone and the supporting evidence for the change was the profiles of low end android devices. I suspect this to be a win across the board.

gaaclarke · 2022-04-25T17:50:35Z

Another follow up question: can we identify other inherited widgets that frequently don't change?

I think there is an opportunity to. The other widgets I identified as candidates are [MediaQuery] and [Theme]. I know less about those and how much they change. Also, making them UbiquitiousInheritedWidgets would mean making the code visible across multiple files and we had some reservations on making this a public API so I avoided it since 20ms was a big enough win.

gaaclarke · 2022-04-25T17:52:38Z

I have profiles of the startup time of customer: money on low end Android devices which show it taking up 20ms of startup time in this bookkeeping as well. Details can be found on b/229707977.

Does this change translate to a 20ms faster startup time for customer:money then?

I didn't run the test for customer:money since it is a bit involved. I felt I had enough evidence in hand to know it would be beneficial and I'd get the exact results for them as part of their regular testing.

jonahwilliams · 2022-04-25T17:57:35Z

MediaQuery can change a lot when things like the keyboard opens or screen resizes, I wouldn't risk it

jonahwilliams

Approach LGTM, leave it up to @goderbauer to review the details.

goderbauer

Overall this LGTM.

Please leave a little more breadcrumbs in the docs so a future reader has an easier time understanding what the optimization is and why we did it.

goderbauer · 2022-04-25T18:10:30Z

Maybe leave that as a comment here for future readers, who want to understand why we did this.

goderbauer · 2022-04-25T18:11:46Z

I would just go with the assert here for now.

goderbauer · 2022-04-25T18:13:05Z

nit: nit formatting here should be:

Suggested change

final _TestInheritedElement ancestor = _TestInheritedElement(

const Directionality(

textDirection: TextDirection.ltr, child: Placeholder()));

final _TestInheritedElement ancestor = _TestInheritedElement(

const Directionality(

textDirection: TextDirection.ltr,

child: Placeholder(),

),

);

This and the next nit are still unaddressed.

goderbauer · 2022-04-25T18:13:23Z

goderbauer

LGTM with two remaining nits

goderbauer · 2022-04-25T19:54:05Z

This and the next nit are still unaddressed.

…formance. (flutter/flutter#102336)

flutter#102336)

flutter-dashboard Bot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 21, 2022

gaaclarke commented Apr 21, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/basic.dart Outdated

gaaclarke requested a review from jonahwilliams April 21, 2022 21:34

gaaclarke removed the request for review from jonahwilliams April 22, 2022 18:00

gaaclarke marked this pull request as draft April 22, 2022 18:00

gaaclarke force-pushed the directionality-faster branch 2 times, most recently from a0b5c0c to 7fb7ce9 Compare April 22, 2022 21:38

gaaclarke marked this pull request as ready for review April 22, 2022 22:14

gaaclarke requested a review from jonahwilliams April 22, 2022 22:14

jonahwilliams requested a review from goderbauer April 25, 2022 17:08

goderbauer reviewed Apr 25, 2022

View reviewed changes

jonahwilliams approved these changes Apr 25, 2022

View reviewed changes

goderbauer reviewed Apr 25, 2022

View reviewed changes

gaaclarke requested a review from goderbauer April 25, 2022 18:59

goderbauer approved these changes Apr 25, 2022

View reviewed changes

gaaclarke added 2 commits April 25, 2022 13:26

Made Directionality forego dependency tracking for better performance.

e9e32d2

added test

2360885

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

18713e0 Made Directionality forego dependency tracking for better per…

15cf062

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 9fbdd4c8d50b (104 revisions) flutter-team-archive/plugins#5409

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

9c91b41

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to d6ea2c67d72e (105 revisions) flutter-team-archive/plugins#5411

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

9624abe

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 0b80f08cf058 (106 revisions) flutter-team-archive/plugins#5412

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

b22a484

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 07f1c2047465 (107 revisions) flutter-team-archive/plugins#5413

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

fdc2770

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 4cf60f7659cb (108 revisions) flutter-team-archive/plugins#5414

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

18fca29

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 7363b29c7f83 (109 revisions) flutter-team-archive/plugins#5415

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

c0edea8

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 3d148821ebfd (110 revisions) flutter-team-archive/plugins#5417

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

6f87122

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 9876b0d6d6a2 (111 revisions) flutter-team-archive/plugins#5418

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

5fb08d3

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to bceebba82aec (112 revisions) flutter-team-archive/plugins#5419

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

ce96550

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 9644aa8d7647 (115 revisions) flutter-team-archive/plugins#5420

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

ca8ef01

…formance. (flutter/flutter#102336)

engine-flutter-autoroll mentioned this pull request Apr 27, 2022

Roll Flutter from 1b58a593deac to 0f397c08dc99 (116 revisions) flutter-team-archive/plugins#5421

Closed

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

18713e0 Made Directionality forego dependency tracking for better per…

124f5b8

…formance. (flutter/flutter#102336)

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

18713e0 Made Directionality forego dependency tracking for better per…

0e51aac

…formance. (flutter/flutter#102336)

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

18713e0 Made Directionality forego dependency tracking for better per…

9d7be53

…formance. (flutter/flutter#102336)

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

18713e0 Made Directionality forego dependency tracking for better per…

e4ba70e

…formance. (flutter/flutter#102336)

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

18713e0 Made Directionality forego dependency tracking for better per…

689228c

…formance. (flutter/flutter#102336)

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

18713e0 Made Directionality forego dependency tracking for better per…

0dfe8e5

…formance. (flutter/flutter#102336)

gaaclarke mentioned this pull request May 3, 2022

Make Theme a UbiquitousInheritedWidget #102993

Open

egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022

Made Directionality forego dependency tracking for better performance. (

9ba804f

flutter#102336)

-    final _TestInheritedElement ancestor = _TestInheritedElement(
-        const Directionality(
-            textDirection: TextDirection.ltr, child: Placeholder()));
+    final _TestInheritedElement ancestor = _TestInheritedElement(
+      const Directionality(
+        textDirection: TextDirection.ltr,
+        child: Placeholder(),
+      ),
+    );

Uh oh!

Conversation

gaaclarke commented Apr 21, 2022 • 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 Apr 21, 2022

Uh oh!

Uh oh!

gaaclarke commented Apr 22, 2022

Uh oh!

gaaclarke commented Apr 22, 2022

Uh oh!

jonahwilliams commented Apr 25, 2022

Uh oh!

goderbauer commented Apr 25, 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!

gaaclarke commented Apr 25, 2022

Uh oh!

goderbauer commented Apr 25, 2022

Uh oh!

jonahwilliams commented Apr 25, 2022

Uh oh!

jonahwilliams commented Apr 25, 2022

Uh oh!

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

Uh oh!

Uh oh!

gaaclarke commented Apr 25, 2022

Uh oh!

gaaclarke commented Apr 25, 2022

Uh oh!

jonahwilliams commented Apr 25, 2022

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gaaclarke commented Apr 21, 2022 •

edited

Loading

goderbauer commented Apr 25, 2022 •

edited

Loading

gaaclarke commented Apr 25, 2022 •

edited

Loading