New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web] Speed up PageView/CustomPainter rendering #22408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// | ||
/// Used for web optimization of physical shape represented as | ||
/// a persistent div. | ||
ui.Rect? get webOnlyPathAsLine => pathRef.getLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need the webOnly
prefix as it's in the private _engine
library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
/// Returns horizontal/vertical line bounds or null if not a line. | ||
ui.Rect? getLine() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd give it a more specific name. When I read the call site I was immediately confused "What if the line is neither horizontal nor vertical?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if ((x2 - x3) != width || (y3 - y0) != height) { | ||
return null; | ||
} | ||
return ui.Rect.fromLTWH(x0, y0, width, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check that the path is closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added warning to API, not to use for strokes only fills/clips.
final double y0 = atPoint(0).dy; | ||
final double x1 = atPoint(1).dx; | ||
final double y1 = atPoint(1).dy; | ||
if (_fVerbs[1] != SPath.kLineVerb || y1 != y0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we only detect rects that begin with a horizontal border?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. added comment.
@@ -1537,6 +1537,12 @@ class SurfacePath implements ui.Path { | |||
/// a persistent div. | |||
ui.Rect? get webOnlyPathAsRect => pathRef.getRect(); | |||
|
|||
/// Detects if path is a vertical or horizontal line returns LTRB or null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "... line. Returns ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Description
Speed up PageView/CustomPainter rendering.
bench_page_view_scroll_line_through.html.drawFrameDuration.average 2.63x
bench_page_view_scroll_line_through.html.drawFrameDuration.outlierRatio 1.46x
bench_page_view_scroll_line_through.html.totalUiFrame.average 3.29x
Other benchmarks are in 1.0 to 1.07x range.
Related Issues
flutter/flutter#65824
flutter/flutter#62596
Tests
I added the following tests:
page_test.dart
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read [handling breaking changes].