[framework,web] add FlutterTimeline and semantics benchmarks that use it #128366
Conversation
flutter-dashboard
bot
added
a: accessibility
github-actions
bot
added
the
a: text input
yjbanov
changed the title
| Future<void> setUpAll() async { | ||
| FlutterTimeline.collectionEnabled = true; | ||
| super.setUpAll(); | ||
| SemanticsBinding.instance.ensureSemantics(); |
There was a problem hiding this comment.
Does this have the potential of turning semantics on for other benchmarks?
There was a problem hiding this comment.
No, because we reload the page between the benchmarks.
| /// add/remove widgets, but its enough to trigger the framework to recompute | ||
| /// some of the semantics. | ||
| /// | ||
| /// The expect output numbers of this benchmarks should be very small as |
There was a problem hiding this comment.
| /// The expect output numbers of this benchmarks should be very small as | |
| /// The expected output numbers of this benchmarks should be very small as |
| /// The expect output numbers of this benchmarks should be very small as | ||
| /// scrolling a list view should be a matter of shifting some widgets and | ||
| /// updating the projected clip imposed by the viewport. As of June 2023, the | ||
| /// numbers are not great. Semantics consumes >50% of frame time. |
There was a problem hiding this comment.
It seems like the 50% ratio is more important than the absolute numbers. Should we report the ratio instead? i.e. run the same widget with semantics and without semantics, and report the ratio between the two.
There was a problem hiding this comment.
I actually attempted it just to rediscover that our benchmark system is designed to only accept timeline data in microseconds. Maybe we should generalize it to accept other units.
| bool forward = true; | ||
|
|
||
| // A one-off timer is necessary to allow the framework to measure the | ||
| // available scroll extends before the scroll controller can be exercised |
| // A one-off timer is necessary to allow the framework to measure the | ||
| // available scroll extends before the scroll controller can be exercised | ||
| // to change the scroll position. | ||
| Timer.run(() { |
There was a problem hiding this comment.
Should we check for mounted?
There was a problem hiding this comment.
It's already mounted by the time initState is called.
There was a problem hiding this comment.
I mean inside Timer.run(() { ...here... });
There was a problem hiding this comment.
Oh, I don't think we need it. mounted being false inside the timer implies the benchmark ended before pumping any interesting frames we care to measure, which can't happen because WidgetRecorder waits for a minimum number of frames before stopping the benchmark.
| /// This getter is not optimized to be fast. It is assumed that when metrics | ||
| /// are read back, they do not affect the timings of the work being | ||
| /// benchmarked. | ||
| List<double> get elements { |
There was a problem hiding this comment.
Maybe make this a method so it doesn't "feel fast"?
| final List<double> result = <double>[]; | ||
| for (int i = 0; i < _pointer; i++) { | ||
| result.add(_slice[i]); | ||
| } | ||
| _chain.forEach(result.addAll); |
There was a problem hiding this comment.
Does the order of elements matter? If yes, then _chain.forEach(result.addAll) should be moved above the for loop. If no, then let's mention that in the dartdoc.
There was a problem hiding this comment.
It does! Good catch. Fixed, and added a test for it.
| /// | ||
| /// See: | ||
| /// * https://developer.mozilla.org/en-US/docs/Web/API/Performance/now | ||
| double get performanceTimestamp => 1000 * _performance.now(); |
There was a problem hiding this comment.
Is there a reason performanceTimestamp is a double instead of 1000 * _performance.now().toInt()?
There was a problem hiding this comment.
I tried, but since there's no int64 or Int64List on the web, I have to store these as doubles anyway. Otherwise, I'd have to store values in List<int> (which on the web is an array of Numbers, i.e. double) and lose some performance predictability. This code relies on the fact that I can allocate small fixed-sized Float64List instances for a constant cost.
There was a problem hiding this comment.
Ah, I was confused by this line:
static int get now => impl.performanceTimestamp.toInt();But I see that it's being retained as a double elsewhere.
| /// | ||
| /// When disabled, resets collected data by calling [reset]. | ||
| /// | ||
| /// Throws a [StateError] if invoked in release mode. |
There was a problem hiding this comment.
This part doesn't seem to be true? It should be, though!
There was a problem hiding this comment.
Good catch! I removed the assertion to debug something, and forgot to put it back.
| /// When disabled, resets collected data by calling [reset]. | ||
| /// | ||
| /// Throws a [StateError] if invoked in release mode. | ||
| static set collectionEnabled(bool value) { |
There was a problem hiding this comment.
call this debugCollectionEnabled to make clear that this is non-release only functionlaity.
|
|
||
| static bool _collectionEnabled = false; | ||
|
|
||
| /// Start a synchronous operation labeled [name]. |
There was a problem hiding this comment.
use backticks for arguments that are not also a property: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#dartdoc-specific-requirements
(here and everywhere else)
|
|
||
| /// Returns timings collected since [collectionEnabled] was set to true, or | ||
| /// since the previous [reset], whichever was last. | ||
| static AggregatedTimings collect() { |
There was a problem hiding this comment.
call this debugCollect and document that it can only be called in non-release mode.
| /// Forgets all previously collected timing data. | ||
| /// | ||
| /// This method can be used to break up the data by frames. To do that, call | ||
| /// [collect] at the end of the frame, then call [reset] so that the next |
There was a problem hiding this comment.
Just wondering if collect should just always imply reset? Is there reason to collect, wait a little and then collect again later where you want the previously collected data to be included?
| /// This method can be used to break up the data by frames. To do that, call | ||
| /// [collect] at the end of the frame, then call [reset] so that the next | ||
| /// frame collects independent data. | ||
| static void reset() { |
There was a problem hiding this comment.
should also be called debutReset.
| /// [finishSync] before returning to the event queue. | ||
| static void startSync(String name, { Map<String, Object?>? arguments, Flow? flow }) { | ||
| Timeline.startSync(name, arguments: arguments, flow: flow); | ||
| if (_collectionEnabled) { |
There was a problem hiding this comment.
all these should probably also be guarded by !kReleaseMode to ensure that they are compiled out in release mode to lower the overhead even further.
| aggregate[block.name] = (previousValue.$1 + block.duration, previousValue.$2 + 1); | ||
| } | ||
|
|
||
| aggregatedBlocks = aggregate.entries.map<AggregatedTimedBlock>( |
There was a problem hiding this comment.
Should these maybe only be computed if someone accesses aggregatedBlocks and/or getAggregated?
|
@goderbauer I started adding the
So a few 10s of invocations won't cut into the frame budget, but may provide much better understanding of an app's real performance. Most of the cost comes from LMK which way you prefer to go. I'm open to either. |
I think that's what profile mode is for. It's release mode + hocks to measure performance. We should make this available in profile mode for sure. But I don't see the appeal for release mode. |
Oh, what meant was that apps could collect performance analytics from installed apps in production. But if we're not interested in supporting that, it's fine. That's out of scope for this PR anyway. Just a drive-by thought. The only issue is that the |
| /// Enables metric collection. | ||
| /// | ||
| /// Metric collection can only be enabled in non-release modes. It is most | ||
| /// useful in the profile mode where application performance is representative |
There was a problem hiding this comment.
uber-nit:
| /// useful in the profile mode where application performance is representative | |
| /// useful in profile mode where application performance is representative |
| } | ||
| } | ||
|
|
||
| /// Emit an instant event. |
There was a problem hiding this comment.
nit: add the doc line about this being a drop-in replacement for [Timeline.instantSync]?
| static int get now => impl.performanceTimestamp.toInt(); | ||
|
|
||
| /// Returns timings collected since [debugCollectionEnabled] was set to true, or | ||
| /// since the previous [debugReset], whichever was last. |
There was a problem hiding this comment.
add: ... or the previous debugCollect ... ?
| /// Forgets all previously collected timing data. | ||
| /// | ||
| /// This method can be used to break up the data by frames. To do that, call | ||
| /// [debugCollect] at the end of the frame, then call [debugReset] so that the next |
There was a problem hiding this comment.
update this since debugCollect automatically does a debugReset?
yjbanov
added
the
autosubmit
flutter/flutter@c40baf4...042c036 2023-06-22 ahmedelsaayid@gmail.com Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 tessertaha@gmail.com Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 utisam@gmail.com Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 tessertaha@gmail.com Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 engine-flutter-autoroll@skia.org Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 42216813+eliasyishak@users.noreply.github.com Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 zanderso@users.noreply.github.com Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 jason-simmons@users.noreply.github.com Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 yjbanov@google.com [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#128966) 2023-06-21 6655696+guidezpl@users.noreply.github.com Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 jmccandless@google.com Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 christopherfujino@gmail.com [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 rmolivares@renzo-olivares.dev Selection area right click behavior should match native (flutter/flutter#128224) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC dit@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
FlutterTimeline
Add a new class
FlutterTimelinethat's a drop-in replacement forTimelinefromdart:developer. In addition to forwarding invocations ofstartSync,finishSync,timeSync, andinstantSynctodart:developer, provides the following extra methods that make is easy to collect timings for code blocks on a frame-by-frame basis:debugCollect()- aggregates timings since the last reset, or since the app launched.debugReset()- forgets all data collected since the previous reset, or since the app launched. This allows clearing data from previous frames so timings can be attributed to the current frame.now- this was enhanced so that it works on the web by callingwindow.performance.now(inTimelinethis is a noop in Dart web compilers).collectionEnabled- a field that controls whetherFlutterTimelinestores timings in memory. By default this is disabled to avoid unexpected overhead (although the class is designed for minimal and predictable overhead). Specific benchmarks can enable collection to report to Skia Perf.Semantics benchmarks
Add
BenchMaterial3Semanticsthat benchmarks the cost of semantics when constructing a screen full of Material 3 widgets from nothing. It is expected that semantics will have non-trivial cost in this case, but we should strive to keep it much lower than the rendering cost. This is the case already. This benchmark shows that the cost of semantics is <10%.Add
BenchMaterial3ScrollSemanticsthat benchmarks the cost of scrolling a previously constructed screen full of Material 3 widgets. The expectation should be that semantics will have trivial cost, since we're just shifting some widgets around. As of today, the numbers are not great, with semantics taking >50% of frame time, which is what prompted this PR in the first place. As we optimize this, we want to see this number improve.