-
Notifications
You must be signed in to change notification settings - Fork 6k
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] Fix canvas leak when dpi changes. Evict from BitmapCanvas cache under… #16721
Conversation
… memory pressure.
@@ -19,6 +19,15 @@ const int _kCanvasCacheSize = 30; | |||
/// Canvases available for reuse, capped at [_kCanvasCacheSize]. | |||
final List<BitmapCanvas> _recycledCanvases = <BitmapCanvas>[]; | |||
|
|||
/// Reduces recycled canvas list by 50% to reduce bitmap canvas memory use. | |||
void _reduceCanvasMemoryUsage() { |
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.
Comment seems inaccurate. It looks like we're actually deleting all of the recycled canvases.
Maybe this would be a little faster with:
for (int i = 0; i < canvasCount; i++) {
_recycledCanvases[i].dispose();
}
_recycledCanvases.clear();
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.
thx. done.
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.
But, is this function used anywhere? :)
@@ -285,7 +299,12 @@ class PersistedStandardPicture extends PersistedPicture { | |||
|
|||
final bool fits = candidate.doesFitBounds(bounds); | |||
final bool isSmaller = candidatePixelCount < lastPixelCount; | |||
if (fits && isSmaller) { | |||
// If we are about to reuse a huge canvas compared to what we need, | |||
// skip candidate since we will be waiting |
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.
waiting for what?
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.
Let's add tests for these changes.
@@ -19,6 +19,15 @@ const int _kCanvasCacheSize = 30; | |||
/// Canvases available for reuse, capped at [_kCanvasCacheSize]. | |||
final List<BitmapCanvas> _recycledCanvases = <BitmapCanvas>[]; | |||
|
|||
/// Reduces recycled canvas list by 50% to reduce bitmap canvas memory use. | |||
void _reduceCanvasMemoryUsage() { |
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.
But, is this function used anywhere? :)
if (fits && isSmaller) { | ||
// [isTooSmall] is used to make sure that a small picture doesn't | ||
// reuse and hold onto memory of a large canvas. | ||
final bool isTooSmall = isSmaller && |
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.
Let's rename the variable to isCanvasTooBig
. Otherwise, the distinction between isSmaller
and isTooSmall
is unclear.
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 would also move this computation inside the if
block, because if the canvas doesn't fit or is not smaller that the previous canvas, then this computation has no effect (you'd also be able to remove the isSmaller
from this expression).
// reuse and hold onto memory of a large canvas. | ||
final bool isTooSmall = isSmaller && | ||
requestedPixelCount > 1 && | ||
(candidatePixelCount / requestedPixelCount) > 10; |
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.
Are we ok picking up canvases that are 10x bigger than the picture?
_paintBounds.grow(_paintBounds.maxPaintBounds); | ||
_commands.add(PaintDrawColor(color, blendMode)); | ||
drawRect( | ||
_paintBounds.maxPaintBounds, |
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.
We can get NaN
in maxPaintBounds
(see https://github.com/flutter/engine/blob/master/lib/web_ui/lib/src/engine/surface/recording_canvas.dart#L1887)
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.
reverted change.
* 5b0cbbe Roll fuchsia/sdk/core/mac-amd64 from _jvYk... to WZgbp... (flutter/engine#16692) * cbb0ff8 Roll src/third_party/skia c5ff41f2976e..7dfb46e7f397 (20 commits) (flutter/engine#16691) * ab454ea Roll src/third_party/dart 0f141be8bd52..7469b87b042a (9 commits) (flutter/engine#16693) * 1b73784 fix param (flutter/engine#16694) * bdd2cf8 Roll src/third_party/skia 7dfb46e7f397..9baef3593c3c (3 commits) (flutter/engine#16696) * 8f0bbfa Roll src/third_party/skia 9baef3593c3c..ed1ff23c2768 (5 commits) (flutter/engine#16699) * f052d2e Roll fuchsia/sdk/core/linux-amd64 from VHyDa... to YPr0t... (flutter/engine#16701) * 3fe3325 Roll src/third_party/dart 7469b87b042a..e187e42593e8 (11 commits) (flutter/engine#16702) * f83092a Roll src/third_party/skia ed1ff23c2768..a5097354217b (1 commits) (flutter/engine#16703) * 6b3d439 Roll src/third_party/skia a5097354217b..2c2db2762809 (1 commits) (flutter/engine#16704) * fdabcad Enable lazy-async-stacks by-default in all modes (flutter/engine#16556) * 02aa865 Fix the newline on some keyboards (flutter/engine#16560) * cde7d47 Roll src/third_party/dart e187e42593e8..81d4cc6bc99a (3 commits) (flutter/engine#16705) * 14a2b03 Roll fuchsia/sdk/core/mac-amd64 from WZgbp... to 78ZcV... (flutter/engine#16706) * 7b0453e Roll src/third_party/skia 2c2db2762809..9d4e31d6cda5 (1 commits) (flutter/engine#16707) * 5cef6d0 Roll fuchsia/sdk/core/linux-amd64 from YPr0t... to CZTpy... (flutter/engine#16708) * 0ef67b5 opt out dart:ui from nnbd (flutter/engine#16473) * bc4a27f [shell tests] Integrate Vulkan with Shell Tests (flutter/engine#16621) * 2fdba52 Roll src/third_party/skia 9d4e31d6cda5..62076977a0b7 (11 commits) (flutter/engine#16710) * 969cfc1 Roll src/third_party/skia 62076977a0b7..1d589a578ca4 (6 commits) (flutter/engine#16712) * 8eb727e Flush the SkCanvas when submitting a frame in ShellTestPlatformViewVulkan::OffscreenSurface (flutter/engine#16717) * e44f99b Roll src/third_party/skia 1d589a578ca4..706851dc99d9 (2 commits) (flutter/engine#16714) * 60b27fd Reland "Remove usage of Dart_AllocateWithNativeFields" (flutter/engine#16713) * 7a50e4d Roll src/third_party/dart 81d4cc6bc99a..fd20c7b92bb8 (31 commits) (flutter/engine#16716) * 4aaafe0 Roll src/third_party/skia 706851dc99d9..df283d01cabb (3 commits) (flutter/engine#16719) * e18aba3 Refactor of ClaimDartHandle -> AssociateWithDartWrapper (flutter/engine#16720) * 66742b6 Roll src/third_party/skia df283d01cabb..3ffa7af75301 (1 commits) (flutter/engine#16722) * e0303b0 Roll src/third_party/dart fd20c7b92bb8..6ef9131d82c4 (7 commits) (flutter/engine#16723) * 0bd69f9 Roll src/third_party/skia 3ffa7af75301..77521d9e06e8 (2 commits) (flutter/engine#16725) * 3e69286 Roll fuchsia/sdk/core/mac-amd64 from 78ZcV... to iYYAH... (flutter/engine#16726) * 704396b Roll fuchsia/sdk/core/linux-amd64 from CZTpy... to -u-iU... (flutter/engine#16727) * 645d4e6 Roll src/third_party/dart 6ef9131d82c4..5829fc7829d5 (3 commits) (flutter/engine#16728) * b73dfed Roll src/third_party/skia 77521d9e06e8..392846665c40 (1 commits) (flutter/engine#16729) * 2724727 Roll src/third_party/skia 392846665c40..bf5cb0f539e7 (1 commits) (flutter/engine#16730) * ab0dd12 [web] Running safari tests on LUCI (flutter/engine#16715) * e5091a8 Enable Vulkan-related shell unittests on Fuchsia (flutter/engine#16718) * 930a80a Fix some compiler warnings in newer versions of Clang. (flutter/engine#16733) * 941aee3 [web] add comment to skipped safari test (flutter/engine#16737) * 136a057 [web] Rename LineMetrics.text to LineMetrics.displayText (flutter/engine#16734) * afe7968 [web] Paragraph.longestLine doesn't need to check for `isSingleLine` anymore (flutter/engine#16736) * c3f4c1a Migrate Path to AssociateWithDartWrapper (flutter/engine#16753) * d0c2418 Add support for Increase Contrast on iOS (flutter/engine#15343) * 6d2cbb2 Roll src/third_party/skia bf5cb0f539e7..46f5c5f08b61 (2 commits) (flutter/engine#16732) * f758eb9 Roll fuchsia/sdk/core/linux-amd64 from -u-iU... to 3rB22... (flutter/engine#16752) * a4a1f4f Roll fuchsia/sdk/core/mac-amd64 from iYYAH... to 5B5jq... (flutter/engine#16744) * 8caf6d1 Roll src/third_party/skia 46f5c5f08b61..9e8f60534464 (29 commits) (flutter/engine#16754) * 340f855 Roll fuchsia/sdk/core/mac-amd64 from 5B5jq... to g1vJn... (flutter/engine#16755) * 9a9abb7 Roll src/third_party/skia 9e8f60534464..d1c90e10f0ca (1 commits) (flutter/engine#16757) * 2e12cdc Roll src/third_party/skia d1c90e10f0ca..998066127e0d (1 commits) (flutter/engine#16759) * 566cfae Roll src/third_party/skia 998066127e0d..57bc977e124c (3 commits) (flutter/engine#16762) * 73fdff1 Roll fuchsia/sdk/core/linux-amd64 from 3rB22... to PGfiE... (flutter/engine#16763) * ecca175 Roll fuchsia/sdk/core/mac-amd64 from g1vJn... to mcI8X... (flutter/engine#16764) * 4d50517 Roll src/third_party/skia 57bc977e124c..cc5415a8ce36 (1 commits) (flutter/engine#16767) * 237ddb1 Roll src/third_party/skia cc5415a8ce36..1cec4d5e3d92 (2 commits) (flutter/engine#16769) * 8da64e0 Roll src/third_party/dart 5829fc7829d5..c75256299280 (43 commits) (flutter/engine#16770) * 0d87160 Roll src/third_party/skia 1cec4d5e3d92..7d252302268a (2 commits) (flutter/engine#16771) * 1aef7a4 Delete FlutterAppDelegate_Internal.h (flutter/engine#16772) * b87bb0a [web] Fix canvas leak when dpi changes. Evict from BitmapCanvas cache under… (flutter/engine#16721) * e0e24f5 Roll src/third_party/skia 7d252302268a..03b8ab225fd7 (8 commits) (flutter/engine#16773) * 38dc2ea [i18n] Deprecates fuchsia.timezone.Timezone (flutter/engine#16700) * 92abb22 Reland "Lift restriction that embedders may not trample the render thread OpenGL context in composition callbacks." (flutter/engine#16711) * 971122b [web] Reduce the usage of unnecessary lists in pointer binding (flutter/engine#16745) * d670059 [web] Respect maxLines when calculating boxes for a range (flutter/engine#16749) * 5496bb4 Roll src/third_party/skia 03b8ab225fd7..659cc1c90705 (4 commits) (flutter/engine#16774) * bad1fbe Roll src/third_party/dart c75256299280..73f6d15665a3 (9 commits) (flutter/engine#16776) * d2e2cc9 Roll fuchsia/sdk/core/mac-amd64 from mcI8X... to O6w2L... (flutter/engine#16777) * 888a62c Revert "Enable lazy-async-stacks by-default in all modes (#16556)" (flutter/engine#16781)
… under… (flutter#16721) * Fix canvas leak when dpi changes. Evict from BitmapCanvas cache under memory pressure. * optimized _reduceCanvasMemoryUsage * Addressed review comments
…as cache under… (flutter#16721)" This reverts commit f15b08d.
Prevents very small recordings using large canvas(s).
When BitmapCanvas cache is full and canvas allocation fails on iOS devices, empty cache and retry.
Change drawColor to utilize DomCanvas instead of screen size BitmapCanvas allocation.
Optimize drawPath for simple rect and roundrect paths.