[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
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
LGTM with some nitpicks.
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.
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 |
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.
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.
There was a problem hiding this comment.
Maybe make this a method so it doesn't "feel fast"?
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.
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.
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.
There was a problem hiding this comment.
call this debugCollectionEnabled to make clear that this is non-release only functionlaity.
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)
There was a problem hiding this comment.
call this debugCollect and document that it can only be called in non-release mode.
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?
There was a problem hiding this comment.
should also be called debutReset.
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.
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 |
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 |
There was a problem hiding this comment.
nit: add the doc line about this being a drop-in replacement for [Timeline.instantSync]?
There was a problem hiding this comment.
add: ... or the previous debugCollect ... ?
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 [email protected] Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 [email protected] Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 [email protected] Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 [email protected] Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 [email protected] Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 [email protected] Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 [email protected] Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 [email protected] Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 [email protected] Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 [email protected] Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 [email protected] Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 [email protected] Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 [email protected] Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 [email protected] [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 [email protected] Roll pub packages (flutter/flutter#128966) 2023-06-21 [email protected] Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 [email protected] Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 [email protected] [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 [email protected] Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 [email protected] 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 [email protected],[email protected],[email protected] 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
3 participants
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.