Skip to content

Commit

Permalink
[web] remove obsolete object caches; simplify native object management (
Browse files Browse the repository at this point in the history
#40894)

(this is attempt 3; details below)

Remove obsolete object caches and introduce a simpler way to manage
native objects:

* Remove the unused `SynchronousSkiaObjectCache`.
* Introduce new library `native_memory.dart` that's smaller and simpler
than `skia_object_cache.dart`.
* Introduce two types of native object references:
  * `UniqueRef` a reference with a unique Dart object owner.
* `CountedRef` a ref-counted reference with multiple Dart object owners.
* All native references use GC (via `FinalizationRegistry`) as a
back-up.
* The new library removes everything related to object resurrection that
was needed only in browsers that didn't support `FinalizationRegistry`.
All browsers support it now.
* Remove the ad hoc `SkParagraph` cache that predates the introduction
of `Paragraph.dispose`.
* Rewrite `CkParagraph` in terms of `UniqueRef`.
* Rewrite `CkImage` in terms of `CountedRef`; delete `SkiaObjectBox`.

This PR does not migrate all objects from the old
`skia_object_cache.dart` to `native_memory.dart`. That would be too big
of a change. The migration can be done in multiple smaller PRs.

This also removes a few unnecessary relayouts observed in
flutter/flutter#120921, but not all of them
(more details in
flutter/flutter#120921 (comment))

## About attempt 3

More about [attempt 2
here](#40862).

In this attempt 3 I'm replacing the `factory` with a top-level function.
  • Loading branch information
yjbanov authored Apr 4, 2023
1 parent 2cb619d commit 3e5f27d
Show file tree
Hide file tree
Showing 13 changed files with 559 additions and 772 deletions.
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1876,6 +1876,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart + ../../../flutter/LICENSE
Expand Down Expand Up @@ -4460,6 +4461,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.d
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart
Expand Down
1 change: 1 addition & 0 deletions lib/web_ui/lib/src/engine.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export 'engine/canvaskit/layer_scene_builder.dart';
export 'engine/canvaskit/layer_tree.dart';
export 'engine/canvaskit/mask_filter.dart';
export 'engine/canvaskit/n_way_canvas.dart';
export 'engine/canvaskit/native_memory.dart';
export 'engine/canvaskit/noto_font.dart';
export 'engine/canvaskit/painting.dart';
export 'engine/canvaskit/path.dart';
Expand Down
2 changes: 0 additions & 2 deletions lib/web_ui/lib/src/engine/canvaskit/canvas.dart
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ class CkCanvas {
offset.dx,
offset.dy,
);
paragraph.markUsed();
}

void drawPath(CkPath path, CkPaint paint) {
Expand Down Expand Up @@ -1112,7 +1111,6 @@ class CkDrawParagraphCommand extends CkPaintCommand {
offset.dx,
offset.dy,
);
paragraph.markUsed();
}
}

Expand Down
13 changes: 8 additions & 5 deletions lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3392,7 +3392,7 @@ abstract class Collector {
class ProductionCollector implements Collector {
ProductionCollector() {
_skObjectFinalizationRegistry =
SkObjectFinalizationRegistry((SkDeletable deletable) {
createSkObjectFinalizationRegistry((SkDeletable deletable) {
// This is called when GC decides to collect the wrapper object and
// notify us, which may happen after the object is already deleted
// explicitly, e.g. when its ref count drops to zero. When that happens
Expand Down Expand Up @@ -3568,10 +3568,13 @@ extension JsConstructorExtension on JsConstructor {
/// 6. We call `delete` on SkPaint.
@JS('window.FinalizationRegistry')
@staticInterop
class SkObjectFinalizationRegistry {
// TODO(hterkelsen): Add a type for the `cleanup` function when
// native constructors support type parameters.
external factory SkObjectFinalizationRegistry(JSFunction cleanup);
class SkObjectFinalizationRegistry {}

SkObjectFinalizationRegistry createSkObjectFinalizationRegistry(JSFunction cleanup) {
return js_util.callConstructor(
_finalizationRegistryConstructor!.toObjectShallow,
<Object>[cleanup],
);
}

extension SkObjectFinalizationRegistryExtension on SkObjectFinalizationRegistry {
Expand Down
56 changes: 6 additions & 50 deletions lib/web_ui/lib/src/engine/canvaskit/image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import 'canvas.dart';
import 'canvaskit_api.dart';
import 'image_wasm_codecs.dart';
import 'image_web_codecs.dart';
import 'native_memory.dart';
import 'painting.dart';
import 'picture.dart';
import 'picture_recorder.dart';
import 'skia_object_cache.dart';

/// Instantiates a [ui.Codec] backed by an `SkAnimatedImage` from Skia.
FutureOr<ui.Codec> skiaInstantiateImageCodec(Uint8List list,
Expand Down Expand Up @@ -227,52 +227,8 @@ Future<Uint8List> readChunked(HttpFetchPayload payload, int contentLength, WebOn
/// A [ui.Image] backed by an `SkImage` from Skia.
class CkImage implements ui.Image, StackTraceDebugger {
CkImage(SkImage skImage, { this.videoFrame }) {
box = CountedRef<CkImage, SkImage>(skImage, this, 'SkImage');
_init();
if (browserSupportsFinalizationRegistry) {
box = SkiaObjectBox<CkImage, SkImage>(this, skImage);
} else {
// If finalizers are not supported we need to be able to resurrect the
// image if it was temporarily deleted. To do that, we keep the original
// pixels and ask the SkiaObjectBox to make an image from them when
// resurrecting.
//
// IMPORTANT: the alphaType, colorType, and colorSpace passed to
// _encodeImage and to canvasKit.MakeImage must be the same. Otherwise
// Skia will misinterpret the pixels and corrupt the image.
final ByteData? originalBytes = _encodeImage(
skImage: skImage,
format: ui.ImageByteFormat.rawRgba,
alphaType: canvasKit.AlphaType.Premul,
colorType: canvasKit.ColorType.RGBA_8888,
colorSpace: SkColorSpaceSRGB,
);
if (originalBytes == null) {
printWarning('Unable to encode image to bytes. We will not '
'be able to resurrect it once it has been garbage collected.');
return;
}
final int originalWidth = skImage.width().toInt();
final int originalHeight = skImage.height().toInt();
box = SkiaObjectBox<CkImage, SkImage>.resurrectable(this, skImage, () {
final SkImage? skImage = canvasKit.MakeImage(
SkImageInfo(
alphaType: canvasKit.AlphaType.Premul,
colorType: canvasKit.ColorType.RGBA_8888,
colorSpace: SkColorSpaceSRGB,
width: originalWidth.toDouble(),
height: originalHeight.toDouble(),
),
originalBytes.buffer.asUint8List(),
(4 * originalWidth).toDouble(),
);
if (skImage == null) {
throw ImageCodecException(
'Failed to resurrect image from pixels.'
);
}
return skImage;
});
}
}

CkImage.cloneOf(this.box, {this.videoFrame}) {
Expand All @@ -291,9 +247,9 @@ class CkImage implements ui.Image, StackTraceDebugger {
StackTrace get debugStackTrace => _debugStackTrace;
late StackTrace _debugStackTrace;

// Use a box because `SkImage` may be deleted either due to this object
// Use ref counting because `SkImage` may be deleted either due to this object
// being garbage-collected, or by an explicit call to [delete].
late final SkiaObjectBox<CkImage, SkImage> box;
late final CountedRef<CkImage, SkImage> box;

/// For browsers that support `ImageDecoder` this field holds the video frame
/// from which this image was created.
Expand All @@ -305,9 +261,9 @@ class CkImage implements ui.Image, StackTraceDebugger {

/// The underlying Skia image object.
///
/// Do not store the returned value. It is memory-managed by [SkiaObjectBox].
/// Do not store the returned value. It is memory-managed by [CountedRef].
/// Storing it may result in use-after-free bugs.
SkImage get skImage => box.skiaObject;
SkImage get skImage => box.nativeObject;

bool _disposed = false;

Expand Down
216 changes: 216 additions & 0 deletions lib/web_ui/lib/src/engine/canvaskit/native_memory.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:js_interop';
import 'package:meta/meta.dart';

import '../../engine.dart' show Instrumentation;
import '../util.dart';
import 'canvaskit_api.dart';

/// Collects native objects that weren't explicitly disposed of using
/// [UniqueRef.dispose] or [CountedRef.unref].
SkObjectFinalizationRegistry _finalizationRegistry = createSkObjectFinalizationRegistry(
(UniqueRef<Object> uniq) {
uniq.collect();
}.toJS
);

NativeMemoryFinalizationRegistry nativeMemoryFinalizationRegistry = NativeMemoryFinalizationRegistry();

/// An indirection to [SkObjectFinalizationRegistry] to enable tests provide a
/// mock implementation of a finalization registry.
class NativeMemoryFinalizationRegistry {
void register(Object owner, UniqueRef<Object> ref) {
_finalizationRegistry.register(owner, ref);
}
}

/// Manages the lifecycle of a C++ object referenced by a single Dart object.
///
/// It is expected that when the C++ object is no longer needed [dispose] is
/// called.
///
/// To prevent memory leaks, the underlying C++ object is deleted by the GC if
/// it wasn't previously disposed of explicitly.
class UniqueRef<T extends Object> {
UniqueRef(Object owner, T nativeObject, this._debugOwnerLabel) {
_nativeObject = nativeObject;
if (Instrumentation.enabled) {
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Created');
}
nativeMemoryFinalizationRegistry.register(owner, this);
}

T? _nativeObject;
final String _debugOwnerLabel;

/// Returns the underlying native object reference, if it has not been
/// disposed of yet.
///
/// The returned reference must not be stored. I should only be borrowed
/// temporarily. Storing this reference may result in dangling pointer errors.
T get nativeObject {
assert(!isDisposed, 'Native object was disposed.');
return _nativeObject!;
}

/// Returns whether the underlying native object has been disposed and
/// therefore can no longer be used.
bool get isDisposed => _nativeObject == null;

/// Disposes the underlying native object.
///
/// The underlying object may be deleted or its ref count may be bumped down.
/// The exact action taken depends on the sharing model of that particular
/// object. For example, an [SkImage] may not be immediately deleted if a
/// [SkPicture] exists that still references it. On the other hand, [SkPaint]
/// is deleted eagerly.
void dispose() {
assert(!isDisposed, 'A native object reference cannot be disposed more than once.');
if (Instrumentation.enabled) {
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Deleted');
}
final SkDeletable object = nativeObject as SkDeletable;
if (!object.isDeleted()) {
object.delete();
}
_nativeObject = null;
}

/// Called by the garbage [Collector] when the owner of this handle is
/// collected.
///
/// Garbage collection is used as a back-up for the cases when the handle
/// isn't disposed of explicitly by calling [dispose]. It most likely
/// indicates a memory leak or inefficiency in the framework or application
/// code.
@visibleForTesting
void collect() {
if (!isDisposed) {
if (Instrumentation.enabled) {
Instrumentation.instance.incrementCounter('$_debugOwnerLabel Leaked');
}
dispose();
}
}
}

/// Interface that classes wrapping [UniqueRef] must implement.
///
/// Used to collect stack traces in debug mode.
abstract class StackTraceDebugger {
/// The stack trace pointing to code location that created or upreffed a
/// [CountedRef].
StackTrace get debugStackTrace;
}

/// Manages the lifecycle of a C++ object referenced by multiple Dart objects.
///
/// Uses reference counting to manage the lifecycle of the C++ object.
///
/// If the C++ object has a unique owner, use [UniqueRef] instead.
///
/// The [ref] method can be used to increment the refcount to tell this box to
/// keep the underlying C++ object alive.
///
/// The [unref] method can be used to decrement the refcount indicating that a
/// referring object no longer needs it. When the refcount drops to zero the
/// underlying C++ object is deleted.
///
/// In addition to ref counting, this object is also managed by GC. When this
/// reference is garbage collected, the underlying C++ object is automatically
/// deleted. This is mostly done to prevent memory leaks in production. Well
/// behaving framework and app code are expected to rely on [ref] and [unref]
/// for timely collection of resources.
class CountedRef<R extends StackTraceDebugger, T extends Object> {
/// Creates a counted reference.
CountedRef(T nativeObject, R debugReferrer, String debugLabel) {
_ref = UniqueRef<T>(this, nativeObject, debugLabel);
if (assertionsEnabled) {
debugReferrers.add(debugReferrer);
}
assert(refCount == debugReferrers.length);
}

/// The native object reference whose lifecycle is being managed by this ref
/// count.
///
/// Do not store this value outside this class.
late final UniqueRef<T> _ref;

/// Returns the underlying native object reference, if it has not been
/// disposed of yet.
///
/// The returned reference must not be stored. I should only be borrowed
/// temporarily. Storing this reference may result in dangling pointer errors.
T get nativeObject => _ref.nativeObject;

/// The number of objects sharing references to this box.
///
/// When this count reaches zero, the underlying [nativeObject] is scheduled
/// for deletion.
int get refCount => _refCount;
int _refCount = 1;

/// Whether the underlying [nativeObject] has been disposed and is no longer
/// accessible.
bool get isDisposed => _ref.isDisposed;

/// When assertions are enabled, stores all objects that share this box.
///
/// The length of this list is always identical to [refCount].
///
/// This list can be used for debugging ref counting issues.
final Set<R> debugReferrers = <R>{};

/// If asserts are enabled, the [StackTrace]s representing when a reference
/// was created.
List<StackTrace> debugGetStackTraces() {
if (assertionsEnabled) {
return debugReferrers
.map<StackTrace>((R referrer) => referrer.debugStackTrace)
.toList();
}
throw UnsupportedError('');
}

/// Increases the reference count of this box because a new object began
/// sharing ownership of the underlying [nativeObject].
void ref(R debugReferrer) {
assert(
!_ref.isDisposed,
'Cannot increment ref count on a deleted handle.',
);
assert(_refCount > 0);
assert(
debugReferrers.add(debugReferrer),
'Attempted to increment ref count by the same referrer more than once.',
);
_refCount += 1;
assert(refCount == debugReferrers.length);
}

/// Decrements the reference count for the [nativeObject].
///
/// Does nothing if the object has already been deleted.
///
/// If this causes the reference count to drop to zero, deletes the
/// [nativeObject].
void unref(R debugReferrer) {
assert(
!_ref.isDisposed,
'Attempted to unref an already deleted native object.',
);
assert(
debugReferrers.remove(debugReferrer),
'Attempted to decrement ref count by the same referrer more than once.',
);
_refCount -= 1;
assert(refCount == debugReferrers.length);
if (_refCount == 0) {
_ref.dispose();
}
}
}
3 changes: 1 addition & 2 deletions lib/web_ui/lib/src/engine/canvaskit/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
/// false.
///
/// This extra flag is necessary on top of [rawSkiaObject] because
/// [rawSkiaObject] being null does not indicate permanent deletion. See
/// similar flag [SkiaObjectBox.isDeletedPermanently].
/// [rawSkiaObject] being null does not indicate permanent deletion.
bool _isDisposed = false;

/// The stack trace taken when [dispose] was called.
Expand Down
Loading

0 comments on commit 3e5f27d

Please sign in to comment.