Add setting for tracking user-created widget builds#4010
Add setting for tracking user-created widget builds#4010kenzieschmoll merged 4 commits intoflutter:masterfrom
Conversation
| import '../../ui/service_extension_widgets.dart'; | ||
| import 'performance_screen.dart'; | ||
|
|
||
| class EnhanceTracingButton extends StatelessWidget { |
There was a problem hiding this comment.
this class was moved over from performance_screen.dart with a couple changes (marked below)
| SecondaryPerformanceControls.minScreenWidthForTextBeforeScaling, | ||
| extensions: [ | ||
| extensions.profileWidgetBuilds, | ||
| extensions.profileUserWidgetBuilds, |
There was a problem hiding this comment.
this line was added
| customExtensionUi: { | ||
| extensions.profileWidgetBuilds.extension: | ||
| const TrackWidgetBuildsSetting(), | ||
| extensions.profileUserWidgetBuilds.extension: const SizedBox(), | ||
| }, |
There was a problem hiding this comment.
these lines were added
CoderDake
left a comment
CoderDake
left a comment
There was a problem hiding this comment.
I tried my best to look over this, but I think I'm still a bit too green to fully understand and approve this.
That being said I learned a lot reading it, and only found a couple of small things to comment on.
packages/devtools_app/lib/src/ui/service_extension_widgets.dart
Outdated
Show resolved
Hide resolved
| final _trackWidgetBuildsExtensions = [ | ||
| extensions.profileWidgetBuilds, | ||
| extensions.profileUserWidgetBuilds, | ||
| ]; |
There was a problem hiding this comment.
| final _trackWidgetBuildsExtensions = [ | |
| extensions.profileWidgetBuilds, | |
| extensions.profileUserWidgetBuilds, | |
| ]; | |
| final _trackWidgetBuildsExtensions = { | |
| TrackWidgetBuildsScope.all: extensions.profileWidgetBuilds, | |
| TrackWidgetBuildsScope.userCreated: extensions.profileUserWidgetBuilds, | |
| }; |
Having an ordering depending on the enum value index feels scary at first glance. Perhaps changing this to a map, or setting based on the index explicitly could be less comment dependent approaches.
There was a problem hiding this comment.
Using a list can save space since with a map, we are keeping a set of keys in memory that we do not actually need. But for this case, since it is small, I have replaced with a map for improved readability.
|
How expensive is it to enable TWB for the user code? I wonder if DevTools should just enable it as soon as the user opens the Performance page. Maybe this depends on project size? |
|
@InMatrix I did a lot of work to try to make it faster to the point were we could enable it by default. The problem is that anytime I made it faster, I also made the baseline faster. The cost varies project to project but in our benchmarks we were seeing a 17% increase in build time with it on: flutter/flutter#100926 (comment) That makes it not a good candidate to always be on, but maybe you are right that it might make make sense if people are actively looking at performance. |
5 participants
CC @InMatrix @ayanogawa
CC @gaaclarke
Fixes #3966