Finalizables don't keep member finalizables alive

[nielsenko]

Compiling with dart compile exe and running the following program on macOS 12.5 (I believe any posix platform will do) with Dart version 2.17.6, 2.18.0-271.4.beta, or 2.19.0-81.0.dev will cause a crash.

import 'dart:ffi';
import 'dart:io';

typedef Free = NativeFunction<Void Function(Pointer)>;
final free = DynamicLibrary.process().lookup<Free>('free');

final _nativeFinalizer = NativeFinalizer(free);

class A implements Finalizable {
  A() {
    _nativeFinalizer.attach(this, Pointer.fromAddress(1), detach: this, externalSize: 1 << 32); // will crash, if it ever runs
  }
}

class B implements Finalizable {
  final A a;

  B(this.a);
}

Future<void> main() async {
  final b = B(A()); // I would expect b.a to live as long as b
  final l = <int>[];
  Future.doWhile(() {
    l.add(1); // put some pressure on GC
    return true;
  });
  await ProcessSignal.sigint.watch().first;
  // b still alive here, but what about b.a?
}

When I force b to be alive by the end of mains scope, by having B implement Finalizable, then I would also expect b.a of type A (which also implements Finalizable) to be kept alive until the end of mains scope. The above example is designed to crash if b.a is ever reaped, by freeing 0x1.

Is this intensional, and if so, how would I ensure that b.a lives as long as b?

[srawlins]

Tentatively passing on to the VM team; it's just my best guess.

[nielsenko]

I think @mraleph and @dcharkes are the ones to /cc

[dcharkes]

Do you always allocate A and B together? In that case have a single implements Finalizable Dart class that owns both.

We haven't covered the composition case.
If you'd like to cover it manually for now.

class A implements Finalizable {
  A() {
    _nativeFinalizer.attach(this, Pointer.fromAddress(1), detach: this, externalSize: 1 << 32); // will crash, if it ever runs
  }

  @pragma('vm:never-inline')
  keepAlive() {}
}

class B implements Finalizable {
  final A a;

  B(this.a);

  void someMethod(){
    // ...
    a.keepAlive();
  }
}

[nielsenko]

I guess I would wish that any members on a type that implements Finalizable that has a type that itself includes Finalizable are guaranteed to live as long as this. But thank you for the work-around - I'll try that.

[nielsenko]

I reworked my code as follows:

import 'dart:ffi';
import 'dart:io';

typedef Free = NativeFunction<Void Function(Pointer)>;
final free = DynamicLibrary.process().lookup<Free>('free');

final _nativeFinalizer = NativeFinalizer(free);

class A implements Finalizable {
  A() {
    _nativeFinalizer.attach(this, Pointer.fromAddress(1), detach: this, externalSize: 1 << 32); // will crash, if it ever runs
  }

  @pragma('vm:never-inline')
  void keepAlive() {}
}

class B implements Finalizable {
  final A a;

  B(this.a);

  @pragma('vm:never-inline')
  void keepAlive() {
    a.keepAlive();
  }
}

Future<void> main() async {
  final b = B(A()); // I would expect b.a to live as long as b
  final l = <int>[];
  Future.doWhile(() {
    l.add(1); // put some pressure on GC
    return true;
  });
  await ProcessSignal.sigint.watch().first;
  print(b); // b still alive here, but what about b.a?
  // b.keepAlive(); // <-- this will save the day, but defeats the purpose I think
}

but that still crash, unless I add an explicit call to b.keepAlive at the end. Did I misunderstand something?

[nielsenko]

Maybe I should clarify, that "save the day" means we get a OoM first:

Exhausted heap space, trying to allocate 34359738384 bytes.
Unhandled exception:
Out of Memory
#0      _GrowableList._allocateData (dart:core-patch/growable_array.dart)
#1      _GrowableList._grow (dart:core-patch/growable_array.dart)
#2      _GrowableList._growToNextCapacity (dart:core-patch/growable_array.dart)
#3      main.<anonymous closure> (file:///Users/kasper/Projects/mongodb/reproductions/lifetime/bin/lifetime.dart)
#4      Future.doWhile.<anonymous closure> (dart:async/future.dart)
#5      _RootZone.runUnaryGuarded (dart:async/zone.dart)
#6      _RootZone.bindUnaryCallbackGuarded.<anonymous closure> (dart:async/zone.dart)
#7      Future.doWhile (dart:async/future.dart)
#8      main (file:///Users/kasper/Projects/mongodb/reproductions/lifetime/bin/lifetime.dart)
#9      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart)
#10     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart)
lifetime.exe(91115,0x105320580) malloc: *** error for object 0x1: pointer being freed was not allocated
lifetime.exe(91115,0x105320580) malloc: *** set a breakpoint in malloc_error_break to debug
fish: Job 1, './bin/lifetime.exe' terminated by signal SIGABRT (Abort)

as opposed to an immediate crash:

lifetime.exe(91280,0x16f6b3000) malloc: *** error for object 0x1: pointer being freed was not allocated
lifetime.exe(91280,0x16f6b3000) malloc: *** set a breakpoint in malloc_error_break to debug
fish: Job 1, './bin/lifetime.exe' terminated by signal SIGABRT (Abort)

[alexmarkov]

It looks like Finalizable only keeps local variables alive. It doesn't affect optimizations which may remove fields (e.g. tree shaking).

@dcharkes We should probably adjust

sdk/pkg/vm/lib/transformations/type_flow/transformer.dart

Line 752 in 5c59f08

bool retainField(Field f) =>

to retain write-only fields of finalizable types in tree shaker.

[dcharkes]

@nielsenko

print(b);

This calls b.toString(), so toString() in class B should be overridden to also keep field a alive.

unless I add an explicit call to b.keepAlive at the end

That does indeed to the same as overriding toString() in B.

Exhausted heap space, trying to allocate 34359738384 bytes.
Unhandled exception:
Out of Memory
#0      _GrowableList._allocateData (dart:core-patch/growable_array.dart)
#1      _GrowableList._grow (dart:core-patch/growable_array.dart)
#2      _GrowableList._growToNextCapacity (dart:core-patch/growable_array.dart)
#3      main.<anonymous closure> (file:///Users/kasper/Projects/mongodb/reproductions/lifetime/bin/lifetime.dart)

34359738384 = 0x800000010. It looks like MongoDB is adding very many elements to a growable list. 8 GB data memory (and the 16 bytes is maybe the object header). Could you investigate that? If MongoDB is not creating a very large list, could you provide a minimal reproduction that causes this in the VM?

Thanks for the pointer @alexmarkov!

to retain write-only fields of finalizable types in tree shaker.

We only need to retain the fields if they are in Finalizable classes themselves.

However, that also leads to the question of write elimination optimizations in the VM. The VM can eliminate writes under certain circumstances. We should also double check those.

[nielsenko]

@dcharkes The above code don't depend on any MongoDB code. The OoM is expected, since I just keep adding integers to an ordinary list. This is just to provoke a GC run, to expose the finalization call to _nativeFinalizer. There is probably a better way, but hence my comment about OoM being the expected behaviour. What I actually see is the crash from freeing 0x1, not the OoM.

I understand I can use a real method like toString to keep the chain of objects alive, just as well as the pseudo method keepAlive, but I kind of like the idea of the keepAlive method, since it makes it very explicit.

I have actually chosen to make keepAlive an extension method, since (in my tests) it has the same effect, and it allows me to hide the otherwise public keepAlive from the user of our package. Perhaps you can confirm if this just as good, or not?

[dcharkes]

I have actually chosen to make keepAlive an extension method, since (in my tests) it has the same effect, and it allows me to hide the otherwise public keepAlive from the user of our package. Perhaps you can confirm if this just as good, or not?

Yes this is fine, extension methods are basically top level methods with the receiver as first argument, and Finalizable arguments are kept alive.

Quick test: https://dart-review.googlesource.com/c/sdk/+/255123

There is probably a better way.

void TriggerGC(uint64_t count) {
  Dart_ExecuteInternalCommand("gc-now", nullptr);
}

Note that Dart_ExecuteInternalCommand is intentionally undocumented, so I should probably not be telling you. 😄 But it might help you trouble shooting. Please note that an unreachable object is not guaranteed to be collected on a GC (and hence its finalizer will not yet run). We have a generational GC and inter-generational references can keep objects alive for multiple successive GCs until all objects involved are all moved to old-space.

[nielsenko]

Thank you @dcharkes for the confirmation, the nifty TriggerGC trick, and the fast turn around on this issue. As always it has been a pleasure. Do you prefer to keep the issue open?

[dcharkes]

Do you prefer to keep the issue open?

We should keep it open for: Finalizables don't keep member finalizables alive.

[dcharkes]

It looks like Finalizable only keeps local variables alive. It doesn't affect optimizations which may remove fields (e.g. tree shaking).

@dcharkes We should probably adjust

sdk/pkg/vm/lib/transformations/type_flow/transformer.dart

Line 752 in 5c59f08

bool retainField(Field f) =>

to retain write-only fields of finalizable types in tree shaker.

It looks like the field is already retained: https://dart-review.googlesource.com/c/sdk/+/254904/1/pkg/vm/testcases/transformations/ffi/finalizable_member.dart.aot.expect#20
(even without my changes to the TFA).

isMemberUsed returns true, most likely due to _in::reachabilityFence(a); in the constructor.

For cleanliness we might want to add my TFA changes anyway.

I'll investigate why B.a is not being kept alive in this reproduction. I suspect write-elimination in the VM rather than the tree-shaking in the CFE.

[alexmarkov]

@dcharkes Here is the AOT kernel of the original example:

  class B extends core::Object implements ffi::Finalizable {
    constructor •([@vm.inferred-type.metadata=library file:///.../foo.dart::A] foo::A a) → foo::B
      : super core::Object::•() {
      ;
      _in::reachabilityFence(this);
      _in::reachabilityFence(a);
    }
  }

So B.a field was removed by the tree shaker. _in::reachabilityFence(a); doesn't prevent that because it uses parameter, not field.

[dcharkes]

@dcharkes Here is the AOT kernel of the original example:

  class B extends core::Object implements ffi::Finalizable {
    constructor •([@vm.inferred-type.metadata=library file:///.../foo.dart::A] foo::A a) → foo::B
      : super core::Object::•() {
      ;
      _in::reachabilityFence(this);
      _in::reachabilityFence(a);
    }
  }

So B.a field was removed by the tree shaker. _in::reachabilityFence(a); doesn't prevent that because it uses parameter, not field.

I'm unsure how I got a different result. I copied the example (https://dart-review.googlesource.com/c/sdk/+/254904/1/pkg/vm/testcases/transformations/ffi/finalizable_member.dart), run the global transformations (https://dart-review.googlesource.com/c/sdk/+/254904/1/pkg/vm/test/transformations/ffi_test.dart#51) and checked that TFA is running with stepping through the debugger. How did you run the transform?

[alexmarkov]

I just ran the AOT compiler and dumped the intermediate kernel file.

pkg/vm/test/transformations/ffi_test.dart calls runGlobalTransformations without specifying optional parameter treeShakeWriteOnlyFields, which is false by default. So the optimization is disabled and tree shaker didn't remove field B.a in the test.