Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c958e83

Browse filesBrowse files
auto-submit[bot]auto-submit[bot]
auto-submit[bot]
and
auto-submit[bot]
authored
Reverts "Lazy paths and frame object arenas (#168996)" (#170164)
<!-- start_original_pr_link --> Reverts: #168996 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: zanderso <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: Failing in post submit on web_long_running_tests_3_5 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: eyebrowsoffire <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {mdebbar} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: The lifecycle of `Path` objects are currently not managed by the user. That is to say, there is no `dispose` method on path objects and therefore no explicit way to detect when the user is done with the path object and the native-side object can be exposed. As of right now, we use `FinalizationRegistry` to clean up the native-side objects when the dart-side objects are garbage collected. However, this has a number of issues: * Adding objects to the finalization registry actually ends up prolonging their lifetime in V8, since the V8 garbage collector will only collect them in a major GC and not a minor GC once they are registered with the finalization registry. See the following Chrome bug: https://issues.chromium.org/issues/340777103 * We can run into OOM issues where the linear memory of canvaskit/skwasm exceeds 2GB if the collection of paths go on too long. * Even if the paths do get collected by the GC, they often happen infrequently enough that paths over many frames have accumulated and are being collected all at once. This gap can often be dozens or hundreds of frames long, and when collection does occur it is freeing a lot of paths at once, which causes a janky frame. I have seen this take upwards of 800ms on my M1 Macbook Pro. There are some more details in #153678 This PR alleviates this issue by creating a `LazyPath` object. This object is added to an arena that explicitly collects the underlying native objects at the end of each frame. The object also tracks the API calls made to it so that if it is actually used across a frame boundary that we can recreate the native object if it was freed. Running our benchmarks, this has a non-trivial performance cost to building and using these paths (30-50% in a microbenchmark, 3-6% in a broader full app benchmark). However, as a team we've decided that this cost is worth it to avoid OOM issues as well as the non-deterministic jank associated with large collections of these objects. <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
1 parent 894fbf6 commit c958e83
Copy full SHA for c958e83

File tree

Expand file treeCollapse file tree

17 files changed

+79
-1167
lines changed
Filter options
Expand file treeCollapse file tree

17 files changed

+79
-1167
lines changed

‎engine/src/flutter/ci/licenses_golden/licenses_flutter

Copy file name to clipboardExpand all lines: engine/src/flutter/ci/licenses_golden/licenses_flutter
-4Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52156,7 +52156,6 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/semantics.dart + ../../../flutter/LICENS
5215652156
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine.dart + ../../../flutter/LICENSE
5215752157
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/alarm_clock.dart + ../../../flutter/LICENSE
5215852158
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/app_bootstrap.dart + ../../../flutter/LICENSE
52159-
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/arena.dart + ../../../flutter/LICENSE
5216052159
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/browser_detection.dart + ../../../flutter/LICENSE
5216152160
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart + ../../../flutter/LICENSE
5216252161
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart + ../../../flutter/LICENSE
@@ -52218,7 +52217,6 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/js_interop/js_typed_data.dart
5221852217
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/key_map.g.dart + ../../../flutter/LICENSE
5221952218
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart + ../../../flutter/LICENSE
5222052219
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/layers.dart + ../../../flutter/LICENSE
52221-
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/lazy_path.dart + ../../../flutter/LICENSE
5222252220
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/mouse/context_menu.dart + ../../../flutter/LICENSE
5222352221
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/mouse/cursor.dart + ../../../flutter/LICENSE
5222452222
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/mouse/prevent_default.dart + ../../../flutter/LICENSE
@@ -55184,7 +55182,6 @@ FILE: ../../../flutter/lib/web_ui/lib/semantics.dart
5518455182
FILE: ../../../flutter/lib/web_ui/lib/src/engine.dart
5518555183
FILE: ../../../flutter/lib/web_ui/lib/src/engine/alarm_clock.dart
5518655184
FILE: ../../../flutter/lib/web_ui/lib/src/engine/app_bootstrap.dart
55187-
FILE: ../../../flutter/lib/web_ui/lib/src/engine/arena.dart
5518855185
FILE: ../../../flutter/lib/web_ui/lib/src/engine/browser_detection.dart
5518955186
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart
5519055187
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart
@@ -55246,7 +55243,6 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/js_interop/js_typed_data.dart
5524655243
FILE: ../../../flutter/lib/web_ui/lib/src/engine/key_map.g.dart
5524755244
FILE: ../../../flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart
5524855245
FILE: ../../../flutter/lib/web_ui/lib/src/engine/layers.dart
55249-
FILE: ../../../flutter/lib/web_ui/lib/src/engine/lazy_path.dart
5525055246
FILE: ../../../flutter/lib/web_ui/lib/src/engine/mouse/context_menu.dart
5525155247
FILE: ../../../flutter/lib/web_ui/lib/src/engine/mouse/cursor.dart
5525255248
FILE: ../../../flutter/lib/web_ui/lib/src/engine/mouse/prevent_default.dart

‎engine/src/flutter/lib/web_ui/lib/src/engine.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine.dart
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ library engine;
1717

1818
export 'engine/alarm_clock.dart';
1919
export 'engine/app_bootstrap.dart';
20-
export 'engine/arena.dart';
2120
export 'engine/browser_detection.dart';
2221
export 'engine/canvaskit/canvas.dart';
2322
export 'engine/canvaskit/canvaskit_api.dart';
@@ -78,7 +77,6 @@ export 'engine/js_interop/js_typed_data.dart';
7877
export 'engine/key_map.g.dart';
7978
export 'engine/keyboard_binding.dart';
8079
export 'engine/layers.dart';
81-
export 'engine/lazy_path.dart';
8280
export 'engine/mouse/context_menu.dart';
8381
export 'engine/mouse/cursor.dart';
8482
export 'engine/mouse/prevent_default.dart';

‎engine/src/flutter/lib/web_ui/lib/src/engine/arena.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/arena.dart
-22Lines changed: 0 additions & 22 deletions
This file was deleted.

‎engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvaskit_canvas.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvaskit_canvas.dart
+15-9Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,20 @@
44

55
import 'dart:typed_data';
66

7-
import 'package:ui/src/engine.dart';
87
import 'package:ui/ui.dart' as ui;
98

9+
import '../validators.dart';
10+
import '../vector_math.dart';
11+
import 'canvas.dart';
12+
import 'canvaskit_api.dart';
13+
import 'image.dart';
14+
import 'painting.dart';
15+
import 'path.dart';
16+
import 'picture.dart';
17+
import 'picture_recorder.dart';
18+
import 'text.dart';
19+
import 'vertices.dart';
20+
1021
/// An implementation of [ui.Canvas] that is backed by a CanvasKit canvas.
1122
class CanvasKitCanvas implements ui.Canvas {
1223
factory CanvasKitCanvas(ui.PictureRecorder recorder, [ui.Rect? cullRect]) {
@@ -129,7 +140,7 @@ class CanvasKitCanvas implements ui.Canvas {
129140

130141
@override
131142
void clipPath(ui.Path path, {bool doAntiAlias = true}) {
132-
_canvas.clipPath((path as LazyPath).builtPath as CkPath, doAntiAlias);
143+
_canvas.clipPath(path as CkPath, doAntiAlias);
133144
}
134145

135146
@override
@@ -253,7 +264,7 @@ class CanvasKitCanvas implements ui.Canvas {
253264

254265
@override
255266
void drawPath(ui.Path path, ui.Paint paint) {
256-
_canvas.drawPath((path as LazyPath).builtPath as CkPath, paint as CkPaint);
267+
_canvas.drawPath(path as CkPath, paint as CkPaint);
257268
}
258269

259270
@override
@@ -425,11 +436,6 @@ class CanvasKitCanvas implements ui.Canvas {
425436
}
426437

427438
void _drawShadow(ui.Path path, ui.Color color, double elevation, bool transparentOccluder) {
428-
_canvas.drawShadow(
429-
(path as LazyPath).builtPath as CkPath,
430-
color,
431-
elevation,
432-
transparentOccluder,
433-
);
439+
_canvas.drawShadow(path as CkPath, color, elevation, transparentOccluder);
434440
}
435441
}

‎engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.dart
+7-4Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,14 @@
44

55
import 'dart:typed_data';
66

7-
import 'package:ui/src/engine.dart';
87
import 'package:ui/ui.dart' as ui;
98

9+
import '../vector_math.dart';
10+
import 'layer.dart';
11+
import 'layer_tree.dart';
12+
import 'path.dart';
13+
import 'picture.dart';
14+
1015
class LayerScene implements ui.Scene {
1116
LayerScene(RootLayer rootLayer) : layerTree = LayerTree(rootLayer);
1217

@@ -109,9 +114,7 @@ class LayerSceneBuilder implements ui.SceneBuilder {
109114
ui.Clip clipBehavior = ui.Clip.antiAlias,
110115
ui.EngineLayer? oldLayer,
111116
}) {
112-
return pushLayer<ClipPathEngineLayer>(
113-
ClipPathEngineLayer((path as LazyPath).builtPath as CkPath, clipBehavior),
114-
);
117+
return pushLayer<ClipPathEngineLayer>(ClipPathEngineLayer(path as CkPath, clipBehavior));
115118
}
116119

117120
@override

‎engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart
+3-18Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import 'dart:typed_data';
77

88
import 'package:ui/ui.dart' as ui;
99

10-
import '../lazy_path.dart';
10+
import '../scene_painting.dart';
1111
import '../vector_math.dart';
1212
import 'canvaskit_api.dart';
1313
import 'native_memory.dart';
@@ -16,7 +16,7 @@ import 'path_metrics.dart';
1616
/// An implementation of [ui.Path] which is backed by an `SkPath`.
1717
///
1818
/// The `SkPath` is required for `CkCanvas` methods which take a path.
19-
class CkPath implements DisposablePath {
19+
class CkPath implements ScenePath {
2020
factory CkPath() {
2121
final SkPath skPath = SkPath();
2222
skPath.setFillType(toSkFillType(ui.PathFillType.nonZero));
@@ -44,11 +44,6 @@ class CkPath implements DisposablePath {
4444

4545
ui.PathFillType _fillType;
4646

47-
@override
48-
void dispose() {
49-
_ref.dispose();
50-
}
51-
5247
@override
5348
ui.PathFillType get fillType => _fillType;
5449

@@ -160,7 +155,7 @@ class CkPath implements DisposablePath {
160155
}
161156

162157
@override
163-
CkPathMetrics computeMetrics({bool forceClosed = false}) {
158+
ui.PathMetrics computeMetrics({bool forceClosed = false}) {
164159
return CkPathMetrics(this, forceClosed);
165160
}
166161

@@ -315,13 +310,3 @@ class CkPath implements DisposablePath {
315310
return skiaObject.isEmpty();
316311
}
317312
}
318-
319-
class CkPathConstructors implements DisposablePathConstructors {
320-
@override
321-
CkPath createNew() => CkPath();
322-
323-
@override
324-
CkPath combinePaths(ui.PathOperation operation, DisposablePath path1, DisposablePath path2) {
325-
return CkPath.combine(operation, path1 as CkPath, path2 as CkPath);
326-
}
327-
}

‎engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/path_metrics.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/path_metrics.dart
+13-23Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,25 @@
55
import 'dart:collection';
66
import 'dart:typed_data';
77

8-
import 'package:ui/src/engine.dart';
98
import 'package:ui/ui.dart' as ui;
109

11-
class CkPathMetrics extends IterableBase<ui.PathMetric> implements DisposablePathMetrics {
10+
import 'canvaskit_api.dart';
11+
import 'native_memory.dart';
12+
import 'path.dart';
13+
14+
class CkPathMetrics extends IterableBase<ui.PathMetric> implements ui.PathMetrics {
1215
CkPathMetrics(this._path, this._forceClosed);
1316

1417
final CkPath _path;
1518
final bool _forceClosed;
1619

1720
/// The [CkPath.isEmpty] case is special-cased to avoid booting the WASM machinery just to find out there are no contours.
1821
@override
19-
late final DisposablePathMetricIterator iterator =
22+
late final Iterator<ui.PathMetric> iterator =
2023
_path.isEmpty ? const CkPathMetricIteratorEmpty._() : CkContourMeasureIter(this);
2124
}
2225

23-
class CkContourMeasureIter implements DisposablePathMetricIterator {
26+
class CkContourMeasureIter implements Iterator<ui.PathMetric> {
2427
CkContourMeasureIter(this._metrics) {
2528
_ref = UniqueRef<SkContourMeasureIter>(
2629
this,
@@ -29,11 +32,6 @@ class CkContourMeasureIter implements DisposablePathMetricIterator {
2932
);
3033
}
3134

32-
@override
33-
void dispose() {
34-
_ref.dispose();
35-
}
36-
3735
final CkPathMetrics _metrics;
3836
late final UniqueRef<SkContourMeasureIter> _ref;
3937

@@ -45,8 +43,8 @@ class CkContourMeasureIter implements DisposablePathMetricIterator {
4543
int _contourIndexCounter = 0;
4644

4745
@override
48-
CkContourMeasure get current {
49-
final CkContourMeasure? currentMetric = _current;
46+
ui.PathMetric get current {
47+
final ui.PathMetric? currentMetric = _current;
5048
if (currentMetric == null) {
5149
throw RangeError(
5250
'PathMetricIterator is not pointing to a PathMetric. This can happen in two situations:\n'
@@ -73,7 +71,7 @@ class CkContourMeasureIter implements DisposablePathMetricIterator {
7371
}
7472
}
7573

76-
class CkContourMeasure implements DisposablePathMetric {
74+
class CkContourMeasure implements ui.PathMetric {
7775
CkContourMeasure(this._metrics, SkContourMeasure skiaObject, this.contourIndex) {
7876
_ref = UniqueRef<SkContourMeasure>(this, skiaObject, 'PathMetric');
7977
}
@@ -87,16 +85,11 @@ class CkContourMeasure implements DisposablePathMetric {
8785

8886
SkContourMeasure get skiaObject => _ref.nativeObject;
8987

90-
@override
91-
void dispose() {
92-
_ref.dispose();
93-
}
94-
9588
@override
9689
final int contourIndex;
9790

9891
@override
99-
CkPath extractPath(double start, double end, {bool startWithMoveTo = true}) {
92+
ui.Path extractPath(double start, double end, {bool startWithMoveTo = true}) {
10093
final SkPath skPath = skiaObject.getSegment(start, end, startWithMoveTo);
10194
return CkPath.fromSkPath(skPath, _metrics._path.fillType);
10295
}
@@ -118,19 +111,16 @@ class CkContourMeasure implements DisposablePathMetric {
118111
}
119112
}
120113

121-
class CkPathMetricIteratorEmpty implements DisposablePathMetricIterator {
114+
class CkPathMetricIteratorEmpty implements Iterator<ui.PathMetric> {
122115
const CkPathMetricIteratorEmpty._();
123116

124117
@override
125-
CkContourMeasure get current {
118+
ui.PathMetric get current {
126119
throw RangeError('PathMetric iterator is empty.');
127120
}
128121

129122
@override
130123
bool moveNext() {
131124
return false;
132125
}
133-
134-
@override
135-
void dispose() {}
136126
}

‎engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/renderer.dart

Copy file name to clipboardExpand all lines: engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/renderer.dart
+3-5Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -328,17 +328,15 @@ class CanvasKitRenderer implements Renderer {
328328
ui.FilterQuality? filterQuality,
329329
) => CkImageShader(image, tmx, tmy, matrix4, filterQuality);
330330

331-
CkPathConstructors pathConstructors = CkPathConstructors();
332-
333331
@override
334-
ui.Path createPath() => LazyPath(pathConstructors);
332+
ui.Path createPath() => CkPath();
335333

336334
@override
337-
ui.Path copyPath(ui.Path src) => LazyPath.fromLazyPath(src as LazyPath);
335+
ui.Path copyPath(ui.Path src) => CkPath.from(src as CkPath);
338336

339337
@override
340338
ui.Path combinePaths(ui.PathOperation op, ui.Path path1, ui.Path path2) =>
341-
LazyPath.combined(op, path1 as LazyPath, path2 as LazyPath);
339+
CkPath.combine(op, path1, path2);
342340

343341
@override
344342
ui.TextStyle createTextStyle({

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.