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

Expose WeakRef and finalization tracking APIs in Dart #1847

Closed
jacob314 opened this issue Mar 24, 2021 · 92 comments
Closed

Expose WeakRef and finalization tracking APIs in Dart #1847

jacob314 opened this issue Mar 24, 2021 · 92 comments

Comments

@jacob314
Copy link
Member

jacob314 commented Mar 24, 2021

Admin comment by @mit-mit:

This request is being tracked as a feature via issue dart-lang/sdk#47772.

For details, see the working proposal feature specification:
https://github.com/dart-lang/language/blob/master/accepted/future-releases/1847%20-%20FinalizationRegistry/proposal.md


Original feature request by @jacob314:

The C API for Dart has always exposed this functionality but it was not exposed in the Dart core libraries because it could not be implemented in JavaScript.
The WeakRef proposal has now landed in JavaScript so WeakRef and finalization tracking could now supported in debug Dart builds on all platforms.
Support is available in all major browsers but Safari so the feature would need to be prohibited in release builds until that changes.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
https://github.com/tc39/proposal-weakrefs

Providing this APIs would allow Dart memory leak detection tools to work on both the Dart VM and JavaScript and would simplify integration of leak detection tools into unittests on the VM. For example, without this API, the VMService must be used to detect leaked objects with some performance overhead to find all instances of the LeakedObject class.

The apis could reasonably be exposed in dart:core, dart:collection (as an iterable WeakMap), or dart:developer.
It could make sense to expose this in dart:developer as that would make it less surprising if the APIs work in debug environments but not production environments.

@leafpetersen
Copy link
Member

leafpetersen commented Mar 24, 2021

@mraleph
Copy link
Member

mraleph commented Mar 25, 2021

This feature is also beneficial for dart:ffi use cases - it would allow us to have finalization mechanism without designing a custom one /cc @dcharkes @mkustermann

@jacob314
Copy link
Member Author

Fyi @ferhatb who may have an idea when Safari will support this functionality.

@nshahan
Copy link

nshahan commented Mar 30, 2021

This sounds like it could be a useful addition to dart:developer. Adding to dart:core or dart:collection makes it harder for dart2js if the support from safari is still not there.

Are you proposing the same API as the JavaScript version or some other API that we need to adapt to what is now available in JavaScript?

@mraleph
Copy link
Member

mraleph commented Mar 31, 2021

Safari Technology Preview 14.1 Release 118 has the support for both WeakRef and FinalizationRegistry. So we are almost there.

@jacob314
Copy link
Member Author

I don't see any reason to make the WeakRef API more than a strongly typed extension of the JS APIs. FinalizationRegistry could alternately be implemented by support destructors/finalizers in the Dart language but given we don't want to encourage widespread use of destructors as part of normal object lifecycles, supporting via an api like FinalizationRegistry seems cleaner.

The JS APIs with reified types added seems like a reasonable enough way to implement this barring a case that would make it inefficient to implement on the VM.

Strawman Dart API:

Reference TypeScript API:
https://github.com/microsoft/TypeScript/blob/master/lib/lib.esnext.weakref.d.ts

// This does not have a generic type T in JavaScript but does in TypeScript.
class WeakRef<T> { 
  T? deref();
}

 // This class does not have a generic type in JavaScript or TypeScript. The unregister token could also be given a type but that seems overkill.
class FinalizationRegistry<T, V> {
  FinalizationRegistry(void Function(V value) callback);
  void register(T target, V heldValue, {Object unregisterToken}); // made unregisterToken a named parameter.
  void unregister(Object unregisterToken object): void;
}

@mraleph
Copy link
Member

mraleph commented Apr 1, 2021

Notes on the VM requirements for the finalization API.

When VM team was discussing finalizers in the context of FFI one of the main concerns we have spent time trying to address was around premature finalization. The problem can be illustrated by the following example:

/// Class Foo holds native [resource] and registers
/// its instances with a FinalizationRegistry which
/// destroys corresponding resources when instance
/// becomes unreachable.
class Foo {
  /// Native resource held by the wrapper - lifetime
  /// of the wrapper determines lifetime of the [_resource].
  final _resource;
  void methodSync() {
    // [this] might be reclaimed by GC after evaluating
    // [this.resource] because it is never used again
    // and not kept live
    useResource(this._resource, gc());
  }

  void methodAsync() async {
    // [this] might be reclaimed by GC after evaluating
    // [this.resource] because it is never used again
    // and not kept live
    useResource(this._resource, await gc());
  }
} 

We want to guarantee that it is always safe to interact with _resource within methods of Foo.

The problem in both methodSync and methodAsync above is that this._resource is evaluated before gc occurs and there is nothing else in the method bodies which guarantees liveness of the object pointed by this variable (as the variable itself is never accessed after this._resource is evaluated and thus can be considered dead by the optimizing compiler).

We think this lack of liveness guarantees makes rather hard to write reliable code.

This problem can be addressed in several ways:

  1. Following ECMAScript specification we could specify that finalization callbacks are not allowed to interrupt execution of the synchronous code (see https://tc39.es/ecma262/2021/#sec-host-cleanup-finalization-registry). This would address the problem with methodSync, but methodAsync would still remain broken. Additionally this would prevent users from using FinalizationRegistry for automatically managing reclamation of native resources in their synchronous code - which seems to be a rather strong drawback.
  2. We could extend dart:core library with an auxiliary function reachabilityFence(x) which does nothing but tells compiler to treat x as live at least until after the invocation of reachabilityFence(x). Developer would then have to correctly place reachability fences in their code, e.g. modify methodSync like so:
    void methodSync() {
      useResource(this._resource, gc());
      reachabilityFence(this);
    }
    This addresses the problem, but requires tedious manual work from developers.
  3. We could introduce a marker interface Finalizable (name subject to discussion), which comes with the following guarantee:
    /// Marker interface for objects which can be placed in FinalizationRegistry.
    /// If a class implements this interface then within any method
    /// of that class or its subclasses the receiver object is guranteed to be kept 
    /// alive at least until the end of that method.  
    abstract class Finalizable {
    }
    
    abstract class FinalizationRegistry<T extends Finalizable, V> {
    }
    Note that FinalizationRegistry is restricted to only Finalizable objects.

VM team is strongly in favor of design number 3.

As @jacob314 has pointed out this design does not actually prevent developers from using FinalizationRegistries with non-Finalizable objects, because the following work-around is possible:

final finalizationToken = Expando<Finalizable>();

class _Token implements Finalizable {}

void addToRegistry<V>(FinalizationRegistry<Finalizable, V> r, Object o, V v) {
  finalizationToken[o] ??=  _Token();
  r.register(finalizationToken[o], v);
}

@jacob314
Copy link
Member Author

What are the next steps to move forward with implementation on the VM and the Web. I'd like to use this functionality for leak detection tooling so I can warn about leaks without computing a snapshot. The current proposal of providing a Finalizable interface seems fine. If we later decide it is not needed, we can always relax the addToRegistry api to accept any type rather than just Finalizable.

@yjbanov
Copy link

yjbanov commented Apr 30, 2021

Not all browsers support this so it would be nice if this could be feature-detectable. We use FinalizerRegistry with feature-detection through JS-interop in the Flutter Web Engine. It's working fine for us, but it's web-only and not very discoverable for the larger Flutter/Dart community.

dart:developer is probably not the best place for this, as this API is useful in production code, not just during development. It should be part of dart:core as it's a core language/runtime feature.

@natebosch
Copy link
Member

@sigmundch @rakudrama - can you comment on the feasibility of implementing any of the above proposals in dart2js?

@sigmundch
Copy link
Member

I think it's doable on the web. Obviously the easiest option for us is (1) since it's provided for us by the JS engines. I also think (3) may be doable.

To my surprise, dart2js today may be accidentally already ensuring the Finalizable constraints as defined in (3) above. We lower async methods to a single closure modeling the state machine for the entire async method. The closure captures the receiver, and in doing so, it keeps it alive until the closure is tossed after the entire async method completes.

I do have a question about API (3), though

@mraleph - can you clarify why you only need the liveness within instance methods of the receiver? As defined above it seems you are only concerned with retaining the receiver in the middle of a method within its class (subclasses), and I'm curious why wouldn't we need a similar guarantees within other async methods. For example:

class Foo implements Finalizable {
  final resource;
  ...
}

lastUse() async {
  var f = Foo();
  useResource(f.resource, await gc()); // last-use of f
}

Here dart2js will not capture f. We save resource in a captured local, but f is ephemeral wont be available after the await. Couldn't this lead us to a premature finalization?

In summary - I'd be fine adopting API (3) with the current definition, but if we need to generalize it further, I need to take a closer look to see how feasible it is to do it this Q. However, I'm also fine moving forward with a variant of (3) and document that the web may have unspecified behavior around async code at first.

@natebosch @lrhn - any thoughts on where the API should live? I share @yjbanov opinion that this should not be in dart:developer, since I expect this to be used in production as well. That said, I hesitate to add anything to dart:core.

@mraleph
Copy link
Member

mraleph commented May 3, 2021

@sigmundch

@mraleph - can you clarify why you only need the liveness within instance methods of the receiver? As defined above it seems you are only concerned with retaining the receiver in the middle of a method within its class (subclasses), and I'm curious why wouldn't we need a similar guarantees within other async methods.

The assumption is that native resource is private to the wrapper and never leaks outside, so it can only be interacted with inside the method itself. I will update the code example above. We want to provide some safety and not universal safety - so that users can reason about the liveness of the wrappers (they never die while their methods are on the stack), but not provide any stronger guarantees than that. /cc @alexmarkov @dcharkes @mkustermann

@sigmundch
Copy link
Member

Thanks, makes sense!

We are clear to go with approach (3) at this point.

@alexmarkov
Copy link

/cc @rmacnak-google @a-siva

@dcharkes
Copy link
Contributor

dcharkes commented May 3, 2021

This feature is also beneficial for dart:ffi use cases - it would allow us to have finalization mechanism without designing a custom one.

For cross referencing, once this is implemented, dart-lang/sdk#35770 is obsolete.

  1. We could introduce a marker interface Finalizable (name subject to discussion), which comes with the following guarantee:

    /// Marker interface for objects which can be placed in FinalizationRegistry.
    /// If a class implements this interface then within any method
    /// of that class or its subclasses the receiver object is guranteed to be kept 
    /// alive at least until the end of that method.  
    abstract class Finalizable {
    }
    
    abstract class FinalizationRegistry<T extends Finalizable, V> {
    }

    Note that FinalizationRegistry is restricted to only Finalizable objects.

@mraleph what is V here? Should that have a specific function signature for finalization?

In my exploration with a Finalizable interface I ran into the case where I was using two objects that both implemented the Finalizable interface. This required either using double dispatch or a reachability fence (prototype NativeResource=Finalizable).

We could consider keeping the receiver and parameters that are a subtype of Finalizable alive. (That would be both the Utf8Resource and the DatabaseResource in method prepare in the prototype.) Alternatively, we should document the double-dispatch or manual reachability fence patterns.

@rmacnak-google
Copy link

this lack of liveness guarantees

I disagree that such a lack exists. As I have been pointing out for many years in the context of the debugger, and in particular the implementation of closures, it is an illegal optimization to drop references to anything that remains in scope. If object is the receiver or local of an activation that has not yet returned, it is reachable. If an object is in scope of a closure that is reachable, it is reachable. Implementors usually brush this off because, without weak references, it is only visible in the debugger or activation mirrors, and they do not take debugging or mirrors seriously and will cheat on semantics for some performance. Weak references merely expand the ways in which this cheating can get caught.

@mraleph
Copy link
Member

mraleph commented May 3, 2021

I disagree that such a lack exists.

I understand the argument from the perspective of the debugger, however debugger is not really a part of the language (and neither are activation mirrors). "Lack" here purely refers to how the language currently does not require an implementation to guarantee specific lifetime for a value referenced by a variable.

in particular the implementation of closures

This is somewhat tangential, but users actually expect closures to capture less, not more. Current closure implementation is already not safe-for-space and retaining more (just for the sake of the debugger) is going to make it worse.

will cheat on semantics for some performance

I would not classify this as cheating - though I would agree that it worsens UX in debugger. FWIW, I think it would be good to put the number of the table for variable liveness, so I will schedule benchmark run with --no-prune-dead-locals. (Unfortunately this is not going to show actual impact on AOT, because environments are removed from calls there, but at least will show how it affects JIT).

@mraleph
Copy link
Member

mraleph commented May 4, 2021

FWIW, I think it would be good to put the number of the table for variable liveness, so I will schedule benchmark run with --no-prune-dead-locals

The data shows regressions in JIT mode on computationally heavy code with large number of temporary live variables (not unexpected - because we have to spill these variables to keep them around for deopt points - that was the whole motivation for doing this optimisation in the first place), e.g. Mandelbrot -57.65%, Box2DOctane -28.24%, Base64Decoder -12.21%, there are more similar regressions.

dart2js compilation benchmarks are not affected on the other hand.

Getting AOT numbers would require a more involved prototype - but these numbers demonstrate that keeping local variables alive for the duration of their syntactic lifetime (rather than shortening their lifetime to just cover their last use) has noticeable detrimental impact on the performance.

@lrhn
Copy link
Member

lrhn commented May 5, 2021

I disagree that such a lack exists.

As a language designer, I can assure you that the lack of guarantee exists and is quite deliberate.
The language only specifies how an entire self-contained program behaves. It does not provide for ways to access variables other than by referencing them inside the program. And with good reason. A debugger which can change the content of a variable is unsound.

void foo(int? x) {
  if (x != null) {
     something(); // <-debugger break here, change x to null.
     x.toRadixString(16); // Soundness breached. If not careful, this can cause a core-dump in native code.
  }
}

The debugger should probably prohibit assigning null to x there, and it does. Evaluating x = null doesn't change the variable.

There are other optimizations that a language implementation is free to make, like copying a variable's value instead of its reference if there are no assignments to it:

void foo() {
  var x = 42;
  var f = () => x; // Can be implemented by capturing *value* since there are no later assignments to x.
  something();
  print(f()); 
}

If you break at something() and change x to 37, then you invalidate that optimization. (The debugger should probably not allow you to assign to x there. Don't know if this optimization is used at all.)

The existence of a debugger should not prevent these optimizations. If anything, the optimizations should prevent the debugger from violating compilation assumptions. That includes referencing a variable after it's been optimized away by a liveness analysis.

There is no canonical semantics of debugging which requires variables to even exist. You can inline, tree-shake and optimize almost anything away, as long as you can conclusively prove that it makes no difference to the visible behavior of the program.

@jacob314
Copy link
Member Author

jacob314 commented May 5, 2021

These are good topics but this have diverged from the core question of exposing an appropriate WeakRef api in Dart.
I've filed some separate bugs to track specify Dart liveness guarantees and debugger behavior around soundness and variables in socpe.

I agree that dart:developer is the wrong place for the API. I'm fine with either dart:core of perhaps adding a new Dart library just for memory management. Perhaps dart:memory, dart:gc, or dart:finalizer.

@lrhn
Copy link
Member

lrhn commented May 5, 2021

As an API, I see no issues with the JavaScript FinalizationRegister, except that the unregister token must be an "Object".
I assume that would make it invalid for Dart to use a bool, num, Null or String, since they are not JS "object"s, even though they are Dart objects. (Seems functions are accepted. JS BigInts are not, not sure if we wrap those in dart2js, but could be a complication if not.

It's not a completely new exception, we do the same thing for Expando, and for the same reason, so I guess we can keep that up. It's just annoying that it's a restriction we cannot represent in the type system.

So, the Dart version would be:

/// A register of objects which may invoke a callback when those objects become inaccessible.
///
/// The register allows objects to be registered, 
/// and when those objects become inaccessible to the program.
/// the callback passed to the register's constructor *may* be called 
/// with the registration token associated with the object.
///
/// No promises are made that the callback will ever be called, 
/// only that *if* it is called with a finalization token as argument, 
/// at least one object registered in the registry with that finalization token
/// is no longer accessible to the program.
///
/// If the same object is registered in multiple finalization registries, 
/// or registered multiple times in a single registry,
/// and the object becomes inaccessible to the program, 
/// then any number of those registrations may trigger their associated callback. 
/// It will not necessarily be all or none of them.
///
/// Finalization callbacks will happen as *events*, not during execution of other code and
/// not as a microtask, but as high-level events similar to timer events.
abstract class FinalizationRegistry {
  /// Creates a finalization registry with the given finalization callback.
  external factory FinalizationRegistry(void Function(Object? finalizationToken) callback);

  /// Registers [value] for a finalization callback.
  ///
  /// When [value] is no longer accessible to the program,
  /// the registry *may* call its callback function with [finalizationToken] as argument.
  ///
  /// The [value] and [unregisterToken] arguments do not count towards those objects
  /// being accessible to the program.
  /// Both must be non-[num], non-[String], non-[bool] and non-[Null] values.
  ///
  /// Multiple objects may be registered with the same finalization token,
  /// and the same object may be registered multiple times with different, or the same,
  /// finalization token.
  /// The callback may be called at most once per registration,
  /// and not for registrations which have been unregistered since they were registered.
  void register(Object value, Object? finalizationToken, [Object? unregisterToken]);

  /// Unregisters any finalization callbacks registered with [unregisterToken] as unregister-token.
  ///
  /// After unregistering, those callbacks will not happen even if the registered object
  /// becomes inaccessible.
  void unregister(Object? unregisterToken);
}

That seems reasonable.

If this is functionality we promise to provide on all platforms, putting it in dart:core isn't impossible. If we can't support it properly everywhere, including AoT compilation for arbitrary mobile platforms, then we might want a separate library that we can make available or unavailable depending on the target platform.

@mraleph
Copy link
Member

mraleph commented May 5, 2021

@lrhn Lasse, please see concerns around this API outlined in https://github.com/dart-lang/sdk/issues/45455#issuecomment-812172232. You current design unfortunately does not address our concerns about usability of this API: your design is not usable for asynchronous code - and I think this is a huge drawback.

@lrhn
Copy link
Member

lrhn commented May 6, 2021

True, if the this object can be GC'ed while one of its methods are executing (because that method doesn't refer to this any more), then binding up finalization of this._resource on the GC of this is unsound.

The issue is that the finalization of one value is based on the reachability of another value. Unless the first value keeps the second value alive (and not the opposite, like in this example), then it's possible to GC the second value while the first value is still reachable, and then you get premature finalization.
That's a general problem, not unique to this example. The only thing special here is that it's surprising that this can GC'ed while a method is running, and the code doesn't look like an anti-pattern. The _resource isn't obviously leaked, and yet it can survive the container object.

So, why do we not always keep this alive during execution of a method? Would it be a big memory overhead?
It seems better than having to implement Finalizable, which doesn't solve anything if the problem isn't related to a method call.
Or, in other words: Do we think this is the only problem that we need to fix regarding finalization?

The design here doesn't solve, or even address, this problem.
I don't think that something which can be solved by an API like this. We need some liveness promises in that users can depend on separately from the API used to actually trigger callbacks on GC.

It's a very different feel when you have to declare your finalization objects as finalizable, instead of being able to wait for the death of any object. It's more of a framework that you opt in to than a general feature. Not impossible, but different and more restricted.

The alternative (your option 2) is to just be more careful when dealing with object liveness, and ensure that this is alive until the method ends. Easy to forget, and no warning if you do.

@jacob314
Copy link
Member Author

jacob314 commented May 7, 2021

@munificent @jakemac53 what are your thoughts on supporting the Finalizable marker interface @mraleph suggests with static meta programming rather than a language feature and marker interface? My impression is the static meta programming would need to ensure that sync and async methods in the class keep the this object alive until the method terminates. The advantage I can with implementing this via static meta programming is there isn't necessarily a one size fits all solution on what guarantees users want for finalizers as no guarantees will be perfect.

/// Marker interface for objects which can be placed in FinalizationRegistry.
/// If a class implements this interface then within any method
/// of that class or its subclasses the receiver object is guranteed to be kept 
/// alive at least until the end of that method.  
abstract class Finalizable {
}

@mraleph
Copy link
Member

mraleph commented May 7, 2021

@lrhn

So, why do we not always keep this alive during execution of a method? Would it be a big memory overhead?

My concern here would be about code quality in general rather than memory usage: more live values mean higher register pressure and consequently more spills. Note that expanding liveness like has non-local effects, e.g.

class X {
  final f = Y();
  void x() {
    f.y();
  } 
}

void foo(X o) {
  loop {
    o.x()
  }
}

Under normal circumstances this code can be optimised to:

class X {
  final f = Y();
  void x() {
    f.y();
  } 
}

void foo(X o) {
  final f = o.f;
  loop {
    f.y();
  }
}

and o does not need to be alive within the loop. But if we start expanding liveness as described o now is live in the whole loop.

Do we think this is the only problem that we need to fix regarding finalization?

I think this is a problem that we certainly want to avoid to make finalizers safe to use in FFI context. As @dcharkes points out we might want to just expand the proposal: variables of static type which is a subtype of Finalizable have their liveness expanded to cover the whole syntactic scope they are defined in.

@alexmarkov @dcharkes @mkustermann @rmacnak-google - I did not attend initial discussions of finalizers for FFI. Do you remember anything else?

We need some liveness promises in that users can depend on separately from the API used to actually trigger callbacks on GC.

Liveness is not observable in other ways though, so it is not clear if we really need to separate these two things.

@jakemac53
Copy link
Contributor

@jacob314 Current thinking wouldn't allow an interface alone to have effects on any class that implements that interface - each class would have to specifically have a macro applied. I am also not certain what you actually want to do with it - inject references to this at the end of the methods?

@jacob314
Copy link
Member Author

jacob314 commented May 7, 2021

Correct. The macro would conceptually inject a reference to this at the end of each method. I think it is slightly more complex than that as the reference to this need to be written in a way that isn't optimized out.
@leafpetersen suggested the follow possible api

class GuardedResource<T> {
  T _resource;
   // Calling `use(f)` calls `f` with `_resource, and guarantees that `this` is live until the return
   external S use<S>(S callback(T r));
}

The macro would ensure that all methods in a class are written to wrap their body with a call to use.
We could fill the gap between the present and when the macro is available by adding a lint if classes implementing the market interface or using a marker annotation do not wrap each of their method bodies with a call to use.

@jakemac53
Copy link
Contributor

jakemac53 commented May 7, 2021

The macro would ensure that all methods in a class are written to wrap their body with a call to use.

This is a use case that we want to support generally (it allows for things like auto-memoization, analytics, etc). The current proposal doesn't outline exactly how it would be done so we do need to figure that out. It does have the potential to violate the core principle of "not modifying code" (you could modify parameters before forwarding them to the wrapped method, or not even invoke the actual method at all!). But I think a compromise can be reached here, and the use cases justify it.

@sigmundch
Copy link
Member

... As @dcharkes points out we might want to just expand the proposal: variables of static type which is a subtype of Finalizable have their liveness expanded to cover the whole syntactic scope they are defined in.

@mraleph - do I understand that then the example in https://github.com/dart-lang/sdk/issues/45455#issuecomment-830422779 will now be protected?

@sigmundch
Copy link
Member

Regarding the GuardedResource API, could that be valuable even without considering macros? Would that work in lieu of the Finalizable API?

I'm trying to see if we would be simplifying the problem domain if we require a wrapper "guard" object for finalization purposes, and in doing so, we can relax the requirements on compilers/vm to ensure liveness elsewhere.

For example, instead of:

class A implements Finalizable {
   final ExpensiveResource _resource;
   ...
   doSomething() async {
      useResource(this._resource, await gc());
      // ensure `this` is live here
  }
}

main() {
  var a = new A(createExpensiveResource);
  FinalizationRegistry((o) => o.release())
    ..register(a, a._resource);
}

it would turn into something like:

class A { // no marker needed
   final GuardedResource<ExpensiveResource> _gResource; // always stored as a guarded reference.

   doSomething() async {
      await _gResource.use((resource) async { // GuardedResource is guaranteed to be live by the underlying system
          useResource(resource, await gc());
      }
  }
}

main() {
  var g = GuardedResource(createExpensiveResource());
  var a = new A(g);
  FinalizationRegistry((o) => o.release())
    ..register(g, g);  // finalization API requires the inputs to be instaces of a `GuardedResource`
}

Thoughts?

@dvc94ch
Copy link

dvc94ch commented Oct 27, 2021

swift and kotlin support destructors, would be nice to solve this real problem when interfacing c libraries, databases or other things that need to be cleaned up. I guess coming up with fancy naming is the easiest part of this problem?

@lrhn
Copy link
Member

lrhn commented Oct 28, 2021

I'm ok with using the term Finalizer here but I do worry that is getting a bit cute given how it differs from the usage of the word Finalizer in other languages.

Not sure how much it differs. It's a function which get called when an object is no longer reachable.
And since FinalizationRegistry has precisely one callback, it can be said to be that function (plus some logic to register it on objects, but that can, arguably, be said to be separate from the function itself).

I read "registry" as something where you can "register" (store) something, for later access in some way.

FinalizationRegistry sticks there by "Finalization" not being the thing we store (it's a process, not a thing, should it be FinalizerRegistry?). It also worries me that "Finalization"/"Finalizer" does not exist by itself, so we are registering something which is not itself a thing in the language. On the other hand, "FinalizationCallbacKRegistry" is definitely overkill.

You are not looking accessing the thing in the registry yourself, only "deregistering" it, but the GC system is looking things up instead, so I guess that's close enough.

If Flutter uses the concept of "Registry" in other places, then that does count as precedence.
And using the same name as in JavaScript can also be an advantage in itself, as long as it's not too horrible.

I can live with FinalizerRegistry. It's not great, but it's a gnarly and complex concept to begin with.

I guess coming up with fancy naming is the easiest part of this problem?

Hah, never. Or to use a quote:

There are only two hard things in Computer Science: cache invalidation and naming things.

– Phil Karlton`

I guess a GC-aware registry touches on both 😁

@blaugold
Copy link

blaugold commented Nov 4, 2021

Is there going to be a guarantee that Dart finalizers are not just triggered by GC being performed because of memory pressure, but also when the event loop runs out of tasks?

I'm thinking of the case where a finalizer is closing something like a ReceivePort, which left open keeps an isolate running. If the object that the finalizer has been registered for has become unreachable, but there is not enough memory pressure to cause a GC, the isolate never exits, even though it potentially could.

@dnfield
Copy link

dnfield commented Nov 4, 2021

GC can be caused by memory pressure notifications but are normally caused by new allocations.

ReceivePorts that are open won't be GCed anyway though, because they're still live. But the weak ref api could still be used to notify users about uncollected ReceivePorts.

@mraleph
Copy link
Member

mraleph commented Nov 4, 2021

Is there going to be a guarantee that Dart finalizers are not just triggered by GC being performed because of memory pressure, but also when the event loop runs out of tasks?

No. We are not going to guarantee that GC ever runs. The only thing we can guarantee is that if GC runs and if GC discovers that the object is unreachable then it will eventually invoke the finaliser.

If developer want guaranteed prompt finalisation, then developer should structure their code to explicitly destroy objects.

@dvc94ch
Copy link

dvc94ch commented Nov 4, 2021

If developer want guaranteed prompt finalisation, then developer should structure their code to explicitly destroy objects.

in the context of hot reloading in flutter, can't that lead to double frees and use after free?

@dnfield
Copy link

dnfield commented Nov 4, 2021

If you have a resource that might not otherwise get re-created in hot reload, you can make sure you respond to reassemble (on e.g. State or some binding class or whatever else might be appropriate) to recreate that resource.

IOW, if you called some kind of dispose method on an object which makes it unusable, you need to create a new instance of that object when you hot reload.

@mraleph
Copy link
Member

mraleph commented Nov 25, 2021

Proposal has been accepted. Implementation tracking issues have been filed: dart-lang/sdk#47772

@lozn00
Copy link

lozn00 commented Dec 28, 2021

======= Exception caught by scheduler library =====================================================
The following assertion was thrown during a scheduler callback:
Assertion failed: org-dartlang-sdk:///flutter_web_sdk/lib/_engine/engine/canvaskit/skia_object_cache.dart:248:12
!browserSupportsFinalizationRegistry
is not true

When the exception was thrown, this was the stack: 
C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 251:49  throw_
C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 29:3    assertFailed
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/skia_object_cache.dart 248:13    [_doResurrect]
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/skia_object_cache.dart 245:40    get skiaObject
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/canvas.dart 210:33               drawPicture
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 467:17                paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 337:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 537:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 368:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 141:14                paintChildren
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer.dart 154:5                 paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer_tree.dart 56:17            paint
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/canvaskit/layer_tree.dart 99:16            <fn>
C:/b/s/w/ir/cache/builder/src/out/host_debug/flutter_web_sdk/lib/_engine/engine/profiler.dart 37:18    


@fzyzcjy
Copy link

fzyzcjy commented Dec 29, 2021

Hi, I wonder when will Dart finalizers be available (stable)? This is quite helpful for, for example, fzyzcjy/flutter_rust_bridge#243. Thanks!

@dvc94ch
Copy link

dvc94ch commented Dec 29, 2021

I'd checkout https://GitHub.com/cloudpeers/ffi-gen, which handles this for you.

@fzyzcjy
Copy link

fzyzcjy commented Dec 29, 2021

@dvc94ch Interesting. Sounds like similar to https://github.com/fzyzcjy/flutter_rust_bridge/ ;)

@dnfield
Copy link

dnfield commented Dec 29, 2021

I would strongly discourage anyone from using GC to manage non-Dart objects. If you want to manage native object lifetimes, have an explicit method like close or dispose to release native resources. GC might not ever run, or run too late.

@dnfield
Copy link

dnfield commented Dec 29, 2021

However, using this to assert that appropriate cleanup methods were called before GC is good

@fzyzcjy
Copy link

fzyzcjy commented Dec 30, 2021

@dnfield Thanks for the interesting remark!

have an explicit method like close or dispose to release native resources

Then there are all problems of manual memory management... For example, accidentally forget to call dispose, or call dispose when still using it.

GC might not ever run, or run too late.

I will tell Dart VM about the real (native) memory usage of the pointer. Then, if real memory usage is high, will Dart immediately trigger a GC (to also free up those native memory)?

/cc possible related @raphaelrobert, @Desdaemon, fzyzcjy/flutter_rust_bridge#243

@dnfield
Copy link

dnfield commented Dec 30, 2021

The VM can be aware of external object size, but GC pressure comes from new allocations. It's really not that hard to get into a scenario where you have an external object that takes up very close to a ceiling of memory you want to use, but not enough to trigger a GC. Then, new allocations happen quickly and kick off a GC, but you then run out of memory (or file handles, or some other native limited resource) before the GC can finish.

Flutter had this problem with images for example - we were relying on the GC to clean them up, which works pretty often but does not work so well when you try to load larger images in memory constrained environments. The more we tried to make the GC clean this up for us, the worse it got - the GC sometimes couldn't work fast enough (so new images could get allocated before the GC-able ones got cleaned up, leading to OOMs), and we also were artificially running the GC too often (Because it saw these huge objects it thought it was responsible for cleaning up). So now we explicitly and eagerly dispose images/graphics resources, and you can't get into that race anymore (and we don't need as many GCs, which are pretty resource intensive to run).

@fzyzcjy
Copy link

fzyzcjy commented Dec 30, 2021

@dnfield Thank you so much for the insights!

@mit-mit
Copy link
Member

mit-mit commented Apr 28, 2022

Enabled in language version 2.17 and above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests