Proper memory management in Skwasm #42328

eyebrowsoffire · 2023-05-25T22:04:34Z

This ensures that native skwasm objects are cleaned up when their associated dart-side objects are garbage collected.

kevmoo · 2023-05-25T22:10:54Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/filters.dart

  final ImageFilterHandle handle;
  bool _isDisposed = false;

+  void dispose() {


Drive-by-comment: this code is duplicated a LOT – worth making it a mixin or something?

I took a couple different passes at it trying to DRY this out, this is actually more concise than it was in my previous passes. I did try actually factoring this out into a mixin, but there were two things that made it difficult:

(1) Each object has a different cleanup function for its handle, and since a mixin can't have a generative constructor it's hard for the adopting class to pass it in.
(2) The finalization registry is a static on the derived class, so the derived class can access it. I can't put the registry on the mixin because you'd only get one finalization registry for the mixin and not one per derived class.

There are probably clever ways to avoid these issues, but at a certain point I stopped refactoring and figured this is good enough. If you feel strongly, I can go back and try to figure out a way to fold some of this logic into SkwasmObjectWrapper though... maybe...

Looking at the code, it's clear we'd need some funky new language features to generalize this. Nevermind. 😢

SkwasmFinalizationRegistry already takes a reference to the type-specific clean-up function. Maybe this is doable via SkwasmFinalizationRegistry. (see #42328 (comment))

yjbanov · 2023-05-25T22:09:06Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/canvas.dart

-  }
+  // Note that we do not need to deal with the finalizer registry here, because
+  // the underlying native skia object is tied directly to the lifetime of the
+  // associated SkPictureRecorder.


Good point. I wonder if in debug mode we should watch for recorders dropped on the floor and issue an error.

yjbanov · 2023-05-25T22:13:55Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/filters.dart

-    }
-  }
+  static final SkwasmFinalizationRegistry<RawColorFilter> _registry =
+    SkwasmFinalizationRegistry<RawColorFilter>(colorFilterDispose);


Per-type finalization registry is an interesting choice. Why can't we use one registry for all types? I'm not sure what the trade-offs are actually.

Because each type has its own dispose function?

Back to the mixin discussion, you could have a Map<Type, DisposeFunction>. Then you have one registry, but you need to do a map access on every dispose. 🤷

Each handle has a different native disposal function. The alternative would be to have a single finalization registry, but add an extra layer of indirection that wraps the handle and is polymorphic on the dispose method (this is basically what you're doing on the CK side with your Ref objects). For the skwasm case, I found it a little cleaner to just do the per-object finalization registry and not end up with an extra layer of indirection.

yjbanov · 2023-05-25T22:17:50Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/font_collection.dart


-class SkwasmTypeface {
-  SkwasmTypeface(SkDataHandle data) : handle = typefaceCreate(data);
+class SkwasmTypeface implements SkwasmObjectWrapper<RawTypeface> {


Do we even hold onto typefaces? Could we accidentally delete a typeface that's still in use?

The typeface is a shared pointer under the hood, and the font collection hangs onto it. The SkwasmFontCollection does hold onto typeface objects explicitly. We used to need it in order to scan incoming text to find missing code points, but Skia does that for it for us now. However, when we reset the font collection (which is just a debug only situation for now), we re-register the base Roboto font with the new collection, so keeping the typeface around is useful in that scenario (rather than having to recreate it from data again). So I don't know if there is a super important reason to hang onto them in prod right now, but I don't think it hurts anything and makes the debug reset part easier.

Rereading my comment, I'm not sure my first point was clear, but what I mean is that we don't have to worry about the typefaces dangling if we don't hang onto them, because the FontCollection has its own shared reference to each typeface. Our hanging onto it or not hanging onto it won't affect the operation of the FontCollection itself.

yjbanov · 2023-05-25T22:20:34Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/image.dart

-class SkwasmImage implements ui.Image {
-  SkwasmImage(this.handle);
+class SkwasmImage implements SkwasmObjectWrapper<RawImage>, ui.Image {
+  SkwasmImage(this.handle) {


According to the clone() method below, it is the responsibility of the caller of the constructor to "pre-bump" the ref count of the handle. Does that hold for all invocations of this constructor?

Basically yes. This constructor (and all skwasm object wrappers constructors that just take a raw handle) expect a handle that has +1 ref count on it, and the object basically takes ownership of that ref count. All the native create functions return a handle with +1 ref count on it, and when cloning we bump the ref count for this same reason.

yjbanov · 2023-05-25T22:22:44Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/memory.dart

+
+typedef DisposeFunction<T extends NativeType> = void Function(Pointer<T>);
+
+class SkwasmFinalizationRegistry<T extends NativeType> {


Do we actually need the <T> type parameter? If not, then I think we can use a shared DomFinalizationRegistry instance for all objects. But, again, I'm not sure what kind of tradeoff exists in using one vs many registries 🤷 Just pointing out a possibility.

We do need this type parameter, because each finalization registry has its own disposal function that it invokes and that disposal function takes a specific pointer type. I mentioned this in the other comment that it is possible to do this with another wrapper which basically "type-erases" this problem away, but I'd rather not add another layer of indirection.

yjbanov · 2023-05-25T22:25:01Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paint.dart

+
+  void dispose() {
+    assert(!_isDisposed);
+    _registry.unregister(this);


This pattern is used in all types. I wonder it can be rolled into SkwasmFinalizationRegistry. We could have a nicely symmetric API:

registry.register(this); registry.dispose(this); // also unregisters since it doesn't need to be GC'd any more

yjbanov · 2023-05-25T22:28:02Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/memory.dart

+
+  final DomFinalizationRegistry registry;
+
+  void register(SkwasmObjectWrapper<T> wrapper) {


One nice feature of UniqueRef/CountedRef is that they integrate with Instrumentation which helps you find leaks and double-frees. But maybe this can be added in a follow-up PR, if the design allows it.

kevmoo · 2023-05-25T23:35:42Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/memory.dart

    ).toJS);

  final DomFinalizationRegistry registry;
+  final DisposeFunction<T> dispose;


nit: could these fields be private?

One unusual aspect of our codebase is that everything under src/engine/ is automatically private, because we rewrite the code into dart:_engine. So it's OK to leave things public if it helps with testing and such.

defaulting to private makes it easy to understand what's used. It also helps find when things are no longer used.

kevmoo · 2023-05-25T23:35:50Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/memory.dart

+  final Pointer<T> handle;
+  bool _isDisposed = false;
+
+  void dispose() {


yjbanov

…127642) flutter/engine@b6fcbe3...f63fcf5 2023-05-26 [email protected] Proper memory management in Skwasm (flutter/engine#42328) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

eyebrowsoffire added 2 commits May 25, 2023 15:02

Use finalization registry to collect Skwasm objects.

ba106c0

Generalize finalization logic a bit more.

c26c12a

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 25, 2023

kevmoo reviewed May 25, 2023

View reviewed changes

yjbanov reviewed May 25, 2023

View reviewed changes

eyebrowsoffire added 3 commits May 25, 2023 15:48

Update licenses golden.

fde5c25

Fix path metrics disposal methods.

474759a

Refactor SkwasmObjectWrapper to deduplicate some code.

0b3b56a

kevmoo reviewed May 25, 2023

View reviewed changes

yjbanov approved these changes May 25, 2023

View reviewed changes

Fixed onCreate/onDispose callbacks for picture and image.

7cad729

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label May 26, 2023

auto-submit bot merged commit f63fcf5 into flutter:main May 26, 2023

engine-flutter-autoroll mentioned this pull request May 26, 2023

Roll Flutter Engine from b6fcbe31ba1a to f63fcf5a274e (1 revision) flutter/flutter#127642

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 26, 2023

f63fcf5a2 Proper memory management in Skwasm (flutter/engine#42328)

a291f64


		typedef DisposeFunction<T extends NativeType> = void Function(Pointer<T>);

		class SkwasmFinalizationRegistry<T extends NativeType> {


		final DomFinalizationRegistry registry;

		void register(SkwasmObjectWrapper<T> wrapper) {

Proper memory management in Skwasm #42328

Proper memory management in Skwasm #42328

Uh oh!

Conversation

eyebrowsoffire commented May 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants