[framework] dont allocate forgotten children set in profile/release mode #94911

jonahwilliams · 2021-12-09T04:35:24Z

Running #94585, HashSet constructor shows up in the top 10 or so hot functions. Since this is unused in profile/release mode and private - just set to null.

@dnfield

jonahwilliams · 2021-12-09T04:44:35Z

Also fyi @srawlins , the lint this is hitting seems a bit bogus. I wrote:

final Set<Element>? _debugForgottenChildrenWithGlobalKey = kDebugMode ? HashSet<Element>() : null;

And I can satisfy the lint by switching it to

late final Set<Element>? _debugForgottenChildrenWithGlobalKey = kDebugMode ? HashSet<Element>() : null;

Which seems pointless (and worse IMO).

dnfield · 2021-12-09T05:07:28Z

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

 // [update] to remove the global key reservations of forgotten children.
- final Set<Element> _debugForgottenChildrenWithGlobalKey = HashSet<Element>();
+ // Initialze to null in profile/release mode to avoid allocating unused sets
+ final Set<Element>? _debugForgottenChildrenWithGlobalKey = kDebugMode ? HashSet<Element>() : null;


Why isn't this tree shaken away as a write only field?

🤷 is it write only?

Yes - it's assigned on creation and then never used again outside of asserts. @alexmarkov do you know why this would still show up in profile builds using --aot --tfa?

Do those flags apply in profile mode? (I think so)

Are you sure it's this one here and not the one in _updateInheritence?

Looking at a release snapshot this is definitely shaken away. It looks like it is in the profile one as well if I'm reading the tooling output correctly.

#94955 seems to substantially improve build times on Android Go. Running more benchmarks.

seems like a good deal!

Digging a little more on this, late final does not end up letting the field get tree shaken, but your version here does.

Oh, I think we'd probably want to eliminate both the field and initializer. WDUT about reopening this (and applying it to all the complex fields ?)

jonahwilliams · 2021-12-10T02:17:28Z

I'll close this in favor of Dan's change, thanks all!

jonahwilliams · 2021-12-12T20:31:57Z

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

- lockState(() {
- _inactiveElements._unmountAll(); // this unregisters the GlobalKeys
- });
+ lockState(_inactiveElements._unmountAll); // this unregisters the GlobalKeys


unrelated, can't help myself

jonahwilliams · 2021-12-12T20:33:39Z

Open to ideas for testing, some thoughts:

I could create a private test/release mode test combo that allowed me to assert these fields are null. but, the issue we don't actually care if these fields are null - just that they are eliminated along with their initializers in profile/release mode (and I don't particularly care how that happens).

A more appropriate test might be to analyze a compiled application, but I don't think we have any tools to do that in the SDK currently?

dnfield · 2021-12-12T20:41:38Z

Maybe a test that tracks the size of the new Element constructor from the heap snapshot? That and RenderObject seem like good things to try to drive down, although it wouldn't strictly catch this. I also expect this will show some improvement in our transition perf benchmarks.

jonahwilliams · 2021-12-12T20:48:34Z

benchmarks are good but ideally it would just be a pass/fail signal

dnfield · 2021-12-12T21:42:50Z

Yeah, the only thing I can think of is measuring the size of the Element constructor with and without the change after gen_snapshot does its thing.

jonahwilliams · 2021-12-13T01:31:19Z

I could assert that the size was smaller than it was before this change. that would allow a bit of wiggle room though it would require some explanation in the test as to why it might have failed and how to debug.

goderbauer

This is a cool find!

goderbauer · 2021-12-13T17:55:33Z

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

+
+ // In Profile/Release mode this field is initialized to `null`. The Dart compiler can
+ // eliminate unused fields, but not their initializers.
+ final Set<Element>? _debugIllFatedElements = kDebugMode ? HashSet<Element>() : null;


Interesting. Can we come up with some kind of a check to avoid re-introducing debug only fields that are not initialized to null in release mode? A lint is probably a little to specific?

If you want to maintain it, I could write a regex test... :)

I think a regex test is probably the best we can do right now. We should just add comments explaining what it does and why.

I started writing a regex for this last week and thoguht we'd probably have to do some kind of post filtering on it to avoid it flagging things that are trivial to initialize like bools or ints.

Maybe analyzer package is a better way to do it.

Yeah, ideally we would apply this to all fields with debug in the name. The analysis should be able to detect that something is initialized to a const value and not complain? (At least for the likely simple cases).

At some point we could/should extent the analysis to ensure that debug only fields are only used from asserts and other debugOnly methods to ensure they will actually be shaken out.

Having this opt-in for now is fine, too, though.

I think super ideally the compiler would just eliminate both the field and the initializer and we could delete the test :)

The compiler would have to understand the difference between side effects you want and side effects you don't. But maybe this could be special cased.. It's just a weird perf cliff :\

You could imagine a type that actually does work in its initializer though, and users depend (perhaps unintentionally) on that work getting done.

The bugs @mraleph has filed related to this would be helpful

jonahwilliams · 2021-12-14T16:38:50Z

analysis test added, PTAL

jonahwilliams · 2021-12-14T16:44:32Z

dev/bots/analyze.dart

 final List<File> files = await _gitFiles(workingDirectory);
 for (final File file in files) {
- if (path.basename(file.path).endsWith('_test.dart'))
+ if (path.basename(file.path).endsWith('_test.dart') || path.basename(file.path) == 'analyze.dart')


Without this the check trips on analyze.dart itself if you've changed it

jonahwilliams · 2021-12-14T16:44:42Z

dev/bots/analyze.dart

 }

+const String _kDebugOnlyAnnotation = '@_debugOnly';
+final RegExp _nullInitializedField = RegExp(r'kDebugMode \? [\w\<\> ,{}()]+ : null;');


This wasn't too bad using a Regex

dnfield

LGTM

goderbauer

LGTM

goderbauer · 2021-12-14T18:34:17Z

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

+
+ // In Profile/Release mode this field is initialized to `null`. The Dart compiler can
+ // eliminate unused fields, but not their initializers.
+ final Set<Element>? _debugIllFatedElements = kDebugMode ? HashSet<Element>() : null;


Yeah, ideally we would apply this to all fields with debug in the name. The analysis should be able to detect that something is initialized to a const value and not complain? (At least for the likely simple cases).

At some point we could/should extent the analysis to ensure that debug only fields are only used from asserts and other debugOnly methods to ensure they will actually be shaken out.

Having this opt-in for now is fine, too, though.

…elease mode (#94911)" This reverts commit fda4094.

zanderso · 2022-01-20T00:23:44Z

Due to an infrastructure issue, we're missing transition_perf benchmark data for the range where this commit landed. Also in the same range, the device that the benchmark was run on was changed. To make a long story short, I'm interested in the impact if any that this change had on performance of our benchmarks. Does anyone have any numbers on that handy?

jonahwilliams · 2022-01-20T00:33:06Z

could always revert and reland 🤣

I think @dnfield is the one with the wonky android device that goes really slow and shows the exaggerated impact

jonahwilliams · 2022-01-20T21:09:30Z

@zanderso running the benchmark from #95596 on a pixel 4.

Before this change, average time of 0.9118845 seconds. After this change, average time of 0.7123856 seconds (10 runs).

[framework] dont allocate forgotten children set in profile/release mode

0bd76e2

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 9, 2021

dnfield reviewed Dec 9, 2021

View reviewed changes

This was referenced Dec 9, 2021

Make debug fields late initialized #94955

Closed

proposal: no_write_only_initialization dart-lang/sdk#58590

Open

jonahwilliams closed this Dec 10, 2021

jonahwilliams deleted the unused_set branch December 10, 2021 02:17

jonahwilliams restored the unused_set branch December 11, 2021 18:14

jonahwilliams reopened this Dec 11, 2021

include more fields, make nullable

8df7a1e

jonahwilliams commented Dec 12, 2021

View reviewed changes

goderbauer reviewed Dec 13, 2021

View reviewed changes

This was referenced Dec 14, 2021

consider adding side-effect analysis/annotations to AOT compiler or TFA dart-lang/sdk#47917

Open

consider emitting performance warnings from TFA dart-lang/sdk#47918

Open

Add analysis test

6886647

jonahwilliams marked this pull request as ready for review December 14, 2021 16:38

flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Dec 14, 2021

jonahwilliams commented Dec 14, 2021

View reviewed changes

Update analyze_test.dart

6ae90b3

jonahwilliams requested review from dnfield and goderbauer December 14, 2021 17:29

dnfield approved these changes Dec 14, 2021

View reviewed changes

dnfield added the waiting for tree to go green label Dec 14, 2021

goderbauer approved these changes Dec 14, 2021

View reviewed changes

dnfield merged commit fda4094 into flutter:master Dec 14, 2021

jonahwilliams deleted the unused_set branch December 14, 2021 19:10

dnfield added a commit that referenced this pull request Dec 15, 2021

Revert "[framework] dont allocate forgotten children set in profile/r…

0a919f8

…elease mode (#94911)" This reverts commit fda4094.

dnfield mentioned this pull request Dec 15, 2021

Revert "[framework] dont allocate forgotten children set in profile/release mode" #95347

Closed

[framework] dont allocate forgotten children set in profile/release mode #94911

[framework] dont allocate forgotten children set in profile/release mode #94911

Uh oh!

Conversation

jonahwilliams commented Dec 9, 2021

jonahwilliams commented Dec 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 10, 2021

Choose a reason for hiding this comment

jonahwilliams commented Dec 12, 2021

dnfield commented Dec 12, 2021

jonahwilliams commented Dec 12, 2021

dnfield commented Dec 12, 2021

jonahwilliams commented Dec 13, 2021

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Jan 20, 2022

jonahwilliams commented Jan 20, 2022

jonahwilliams commented Jan 20, 2022

Labels

5 participants