Delete Skia-specific performance overlay implementation #176364
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great simplification, removing the Skia-specific implementation of the performance overlay in favor of the DlStopwatchVisualizer. This not only reduces code duplication but also brings performance improvements. The related changes to the Stopwatch class logic appear to fix existing bugs and improve correctness. The updates to unit tests are thorough, though I have one suggestion to make a test even more robust. Overall, this is a high-quality change.
|
The Google testing failed the merge as there is a conflict vs the most current internal roll that isn't an issue here in the repo. |
|
|
||
| // Crashes reading a nullptr. | ||
| EXPECT_DEATH_IF_SUPPORTED(layer->Paint(paint_context()), ""); | ||
| layer->Paint(paint_context()); |
There was a problem hiding this comment.
Since the test is "does not die" it's worth commenting that this is where we are checking this doesn't die. I wish this method returned an error we could assert on.
There was a problem hiding this comment.
Gemini suggested such a comment, but I thought it was self-evident since the name of the test is "Painting...DoesNotDie"...?
| auto const bar_width = width * sample_unit_width; | ||
| auto const bar_height = height * sample_unit_height; | ||
| auto const bar_left = x + width * sample_unit_width * i; | ||
| auto const bar_top = bottom - sample_unit_height; |
There was a problem hiding this comment.
| auto const bar_top = bottom - sample_unit_height; | |
| const auto bar_top = bottom - sample_unit_height; |
auto const is used a lot in this file but it doesn't match the usage of const across the wider codebase.
Also, please remove the auto. I'm not sure if we are doing floating point math or integer math here.
There was a problem hiding this comment.
Done. There were some cases where we were "auto"ing some doubles that eventually got computed into Scalars which was interesting, but it was one computation per loop. It's explicit now which makes it more obvious, but we're at the mercy of Stopwatch APIs that return doubles and rendering APIs that take floats, so there will always be a conflict there.
There was a problem hiding this comment.
The compiler seems to allow Scalar foo = some_double [op] some_other_double - not sure if we should be explicit about a cast there or just rely on the compiler. It passes clang-tidy...
There was a problem hiding this comment.
Yea that's c++ for yea. I bet there is a flag to turn off implicit casts but would probably be difficult to turn on at this point.
There was a problem hiding this comment.
I think Stopwatch/TimeDelta work with doubles because they are working with "millinano(!)seconds since ?the epoch?" which might exceed the range of float, but I don't think they are using them for sub-unit accuracy. As long as they are a relative value, float is enough, but their absolute values need the range of doubles.
There was a problem hiding this comment.
Yea that's c++ for yea. I bet there is a flag to turn off implicit casts but would probably be difficult to turn on at this point.
Maybe I should set a good example here and make the one conversion explicit...
flutter-dashboard
bot
removed
the
autosubmit
flutter/flutter@7811e89...65aca36 2025-10-02 [email protected] Roll Skia from 257c1f94afaa to 05c1f5803415 (4 revisions) (flutter/flutter#176402) 2025-10-02 [email protected] [ Widget Preview ] Fix resolution for workspace "hosted" dependencies (flutter/flutter#176358) 2025-10-02 [email protected] Roll Skia from b5d8ae8d3410 to 257c1f94afaa (6 revisions) (flutter/flutter#176389) 2025-10-02 [email protected] Delete Skia-specific performance overlay implementation (flutter/flutter#176364) 2025-10-02 [email protected] Roll Fuchsia Linux SDK from 1Ai6VL4vb_GdGnWhg... to Vnoygds8HtDUvGLCK... (flutter/flutter#176381) 2025-10-01 [email protected] [ Widget Preview ] Persist "Filter by Selected File" toggle (flutter/flutter#176289) 2025-10-01 [email protected] Roll Skia from c44a36470d07 to b5d8ae8d3410 (5 revisions) (flutter/flutter#176367) 2025-10-01 [email protected] Reapply "Update the AccessibilityPlugin::Announce method to account f… (flutter/flutter#176107) 2025-10-01 [email protected] Roll Dart SDK from 8ffec1435cf3 to 4f90a06328cb (3 revisions) (flutter/flutter#176369) 2025-10-01 [email protected] [ Tool / l10n ] Fix issue where localization generator assumed current directory was the target project (flutter/flutter#175881) 2025-10-01 [email protected] Make sure that a DateRangePickerDialog doesn't crash in 0x0 environments (flutter/flutter#173754) 2025-10-01 [email protected] Make sure that a DrawerButton doesn't crash in 0x0 environment (flutter/flutter#172948) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
(Essentially a re-issue of flutter#174682 which ended up with broken Google Testing links) We had 2 different implementations of the rendering code for the performance overlay layer. The skia version used some skia-specific code to render the overlay incrementally into an offscreen surface and so we created a different implementation for Impeller that only uses standard rendering calls (and no surface cache). It turns out that the Impeller version was faster anyway even on Skia so it is a simple change to delete the old code and always use the new visualizer. The new visualizer reduces the time to render the graph from just under 1ms to about .1ms on Skia. The new visualizer takes .1ms longer to compute on the UI thread, but overall we save time between the 2 threads. The new visualizer is much faster on Impeller. Some work could be done to save some of that time on the UI thread by only incrementally updating the graph data, but for now we can take the ~.8-.9ms savings with just some deleted code.
(Essentially a re-issue of flutter#174682 which ended up with broken Google Testing links) We had 2 different implementations of the rendering code for the performance overlay layer. The skia version used some skia-specific code to render the overlay incrementally into an offscreen surface and so we created a different implementation for Impeller that only uses standard rendering calls (and no surface cache). It turns out that the Impeller version was faster anyway even on Skia so it is a simple change to delete the old code and always use the new visualizer. The new visualizer reduces the time to render the graph from just under 1ms to about .1ms on Skia. The new visualizer takes .1ms longer to compute on the UI thread, but overall we save time between the 2 threads. The new visualizer is much faster on Impeller. Some work could be done to save some of that time on the UI thread by only incrementally updating the graph data, but for now we can take the ~.8-.9ms savings with just some deleted code.
(Essentially a re-issue of flutter#174682 which ended up with broken Google Testing links) We had 2 different implementations of the rendering code for the performance overlay layer. The skia version used some skia-specific code to render the overlay incrementally into an offscreen surface and so we created a different implementation for Impeller that only uses standard rendering calls (and no surface cache). It turns out that the Impeller version was faster anyway even on Skia so it is a simple change to delete the old code and always use the new visualizer. The new visualizer reduces the time to render the graph from just under 1ms to about .1ms on Skia. The new visualizer takes .1ms longer to compute on the UI thread, but overall we save time between the 2 threads. The new visualizer is much faster on Impeller. Some work could be done to save some of that time on the UI thread by only incrementally updating the graph data, but for now we can take the ~.8-.9ms savings with just some deleted code.
2 participants
(Essentially a re-issue of #174682 which ended up with broken Google Testing links)
We had 2 different implementations of the rendering code for the performance overlay layer. The skia version used some skia-specific code to render the overlay incrementally into an offscreen surface and so we created a different implementation for Impeller that only uses standard rendering calls (and no surface cache). It turns out that the Impeller version was faster anyway even on Skia so it is a simple change to delete the old code and always use the new visualizer.
The new visualizer reduces the time to render the graph from just under 1ms to about .1ms on Skia.
The new visualizer takes .1ms longer to compute on the UI thread, but overall we save time between the 2 threads.
The new visualizer is much faster on Impeller.
Some work could be done to save some of that time on the UI thread by only incrementally updating the graph data, but for now we can take the ~.8-.9ms savings with just some deleted code.