Skip to content
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

Merged
merged 6 commits into from
Feb 24, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions lib/web_ui/lib/src/engine/surface/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx. done.

Copy link
Contributor

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? :)

final int canvasCount = _recycledCanvases.length;
for (int i = 0; i < canvasCount; i++) {
_recycledCanvases[i].dispose();
}
_recycledCanvases.clear();
}

/// A request to repaint a canvas.
///
/// Paint requests are prioritized such that the larger pictures go first. This
Expand All @@ -42,22 +51,27 @@ class _PaintRequest {
List<_PaintRequest> _paintQueue = <_PaintRequest>[];

void _recycleCanvas(EngineCanvas canvas) {
if (canvas is BitmapCanvas && canvas.isReusable()) {
_recycledCanvases.add(canvas);
if (_recycledCanvases.length > _kCanvasCacheSize) {
final BitmapCanvas removedCanvas = _recycledCanvases.removeAt(0);
removedCanvas.dispose();
if (canvas is BitmapCanvas) {
if (canvas.isReusable()) {
_recycledCanvases.add(canvas);
if (_recycledCanvases.length > _kCanvasCacheSize) {
final BitmapCanvas removedCanvas = _recycledCanvases.removeAt(0);
removedCanvas.dispose();
if (_debugShowCanvasReuseStats) {
DebugCanvasReuseOverlay.instance.disposedCount++;
}
}
if (_debugShowCanvasReuseStats) {
DebugCanvasReuseOverlay.instance.disposedCount++;
DebugCanvasReuseOverlay.instance.inRecycleCount =
_recycledCanvases.length;
}
}
if (_debugShowCanvasReuseStats) {
DebugCanvasReuseOverlay.instance.inRecycleCount =
_recycledCanvases.length;
} else {
canvas.dispose();
}
}
}


/// Signature of a function that instantiates a [PersistedPicture].
typedef PersistedPictureFactory = PersistedPicture Function(
double dx,
Expand Down Expand Up @@ -272,7 +286,7 @@ class PersistedStandardPicture extends PersistedPicture {
final ui.Size canvasSize = bounds.size;
BitmapCanvas bestRecycledCanvas;
double lastPixelCount = double.infinity;

final double requestedPixelCount = bounds.width * bounds.height;
for (int i = 0; i < _recycledCanvases.length; i++) {
final BitmapCanvas candidate = _recycledCanvases[i];
if (!candidate.isReusable()) {
Expand All @@ -285,7 +299,12 @@ class PersistedStandardPicture extends PersistedPicture {

final bool fits = candidate.doesFitBounds(bounds);
final bool isSmaller = candidatePixelCount < lastPixelCount;
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 &&
Copy link
Contributor

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.

Copy link
Contributor

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).

requestedPixelCount > 1 &&
(candidatePixelCount / requestedPixelCount) > 10;
Copy link
Contributor

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?

if (fits && isSmaller && !isTooSmall) {
bestRecycledCanvas = candidate;
lastPixelCount = candidatePixelCount;
final bool fitsExactly = candidateSize.width == canvasSize.width &&
Expand Down
25 changes: 21 additions & 4 deletions lib/web_ui/lib/src/engine/surface/recording_canvas.dart
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,12 @@ class RecordingCanvas {
}

void drawColor(ui.Color color, ui.BlendMode blendMode) {
_hasArbitraryPaint = true;
_didDraw = true;
_paintBounds.grow(_paintBounds.maxPaintBounds);
_commands.add(PaintDrawColor(color, blendMode));
drawRect(
_paintBounds.maxPaintBounds,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted change.

ui.Paint()
..color = color
..style = ui.PaintingStyle.fill
..blendMode = blendMode);
}

void drawLine(ui.Offset p1, ui.Offset p2, SurfacePaint paint) {
Expand Down Expand Up @@ -315,6 +317,21 @@ class RecordingCanvas {
}

void drawPath(ui.Path path, SurfacePaint paint) {
if (paint.shader == null) {
// For Rect/RoundedRect paths use drawRect/drawRRect code paths for
// DomCanvas optimization.
SurfacePath sPath = path;
final ui.Rect rect = sPath.webOnlyPathAsRect;
if (rect != null) {
drawRect(rect, paint);
return;
}
final ui.RRect rrect = sPath.webOnlyPathAsRoundedRect;
if (rrect != null) {
drawRRect(rrect, paint);
return;
}
}
_hasArbitraryPaint = true;
_didDraw = true;
ui.Rect pathBounds = path.getBounds();
Expand Down