[ffigen] Some objects must be destroyed on the platform thread

[liamappelbe]

Some ObjC objects can only be used from the platform thread. These objects should also only be destroyed from the platform thread.

We can move our finalizer to the platform thread easily enough, but there's no way of detecting which classes this is necessary for. So there are two options:

1. Move all objc_release calls to the platform thread. This might have performance implications.
2. Add config options for this. This would be pretty inconvenient, and if a user didn't know about this feature, or forgot to opt-in one of their classes, they'd get confusing crashes during GC.

[dcharkes]

"The best API is no API."

So probably option 1, unless we can measure performance impacts.

If we can't do option 1, I'd like to refrain from option 2. Because that would create an asymmetry between runOnPlatfromTread which is in Dart code and an option in ffigen.yaml. But I suppose we can't really make it configurable from Dart?

[stuartmorgan]

1. Move all objc_release calls to the platform thread.

It couldn't be done for all calls. If I have a thread-hostile Obj-C object, and I create an instance of it on a thread other than the platform thread, then this would break things.

[liamappelbe]

If I have a thread-hostile Obj-C object, and I create an instance of it on a thread other than the platform thread, then this would break things.

An object like that will already be broken under the current approach, since GC can run on a background thread. There's currently no guarantee about what thread a finalizer will be run on. Do you have any objects like that?

I guess the ideal solution would be to have some way of ensuring that the object is destroyed on the same thread it was created on. I'm not sure that's possible in general though. We can't even guarantee the creation thread still exists. We might be able to use NativeCallable.listener to send the release call to the isolate that created the object, but that's a very expensive solution and there's no guarantee that the isolate would be running on the same thread.

[stuartmorgan]

Do you have any objects like that?

I don't need this for video_player, no. Having thread-hostile, non-UI objects on background threads is definitely a pattern that can exist in Obj-C though.

There's currently no guarantee about what thread a finalizer will be run on.

That is definitely extremely bad for the common case of platform-thread-only objects. That means my current video_player WIP has undefined behavior, for instance.

[liamappelbe]

I don't see a good way of solving the fully general version of this problem. So until we have a concrete use case where option 1 won't work, I'm inclined to send every release call to the platform thread (assuming this is ok from a performance perspective). We know that this thread exists, and there are existing ObjC constructs to send tasks to it.

If users have an object that is very thread-hostile, my current advice would be to manage its life-cycle in native code (maybe write a thread safe wrapper that can be used in Dart). Or I guess you could just create and interact with it using runOnPlatformThread and rely on option 1 to send the release call to the platform thread. My hunch is that while it's possible to have objects like that in ObjC, they're probably not that common in practice.

[liamappelbe]

Performance doesn't seem to be a significant issue here. I wrote a little benchmark that creates a million NSStrings and converts them back to Dart Strings. This benchmark involves 2 million retains and releases.

final t = Stopwatch()..start();
print('global retain count BEFORE = ${getGlobalRetainCount()}');
void inner() {
  List<NSString>? objs = <NSString>[];
  for (int i = 0; i < 1000000; ++i) {
    objs.add('str $i'.toNSString());
  }
  int lensum = 0;
  for (final o in objs) {
    lensum += o.toString().length;
  }
  print(lensum);
  print('global retain count DURING = ${getGlobalRetainCount()}');
  objs = null;
}
inner();
doGC();
await Future<void>.delayed(Duration(milliseconds: 500));
doGC();
await Future<void>.delayed(Duration(milliseconds: 500));
print('global retain count AFTER = ${getGlobalRetainCount()}');
print('time = ${t.elapsed}');

Under the current approach, this benchmark runs in 3.7 sec (subtracting the 1 second of waiting I'm doing around the GCs). If we dispatch all the releases to the main thread, the benchmark takes 4.1 sec.

That 4.1 sec doesn't include the time spent on the main thread itself, but I don't have a good way of measuring that. One thing I can say is that it definitely spends less than 1 second on the main thread, because by the time that final getGlobalRetainCount() happens, the retain count is 0.

UPDATE: Managed to narrow this down by plotting this retain count over time. It goes from 1 million to 0 in 0.17 sec. That translates to about 0.17 μs spent on the main thread per object.

I'm using this ObjC function to do the dispatching:

void runOnMainThread(void(*fn)(void*), void* arg) {
  dispatch_queue_main_t q = dispatch_get_main_queue();
  if (q == nil) {
    fn(arg);
  } else {
    dispatch_async(q, ^{
      fn(arg);
    });
  }
}

There are 2 issues left to resolve:

1. I need to add an optimization that skips the dispatch if we're already on the main thread. This will be extra important when thread merging lands. Need to research how to detect if we're on the main thread. UPDATE: [NSThread isMainThread] works. Interestingly, just running this benchmark using flutter test, I see this is true a lot of the time. About 34% of the releases are happening directly on the main thread, and the rest have to be dispatched to the main thread.
2. This works in flutter, but for standalone Dart there is no main thread dispatch queue. I was hoping dispatch_get_main_queue() would return null in that case (then I'd fall back to directly calling release), but it seems to always return a valid queue. And since nothing is flushing that queue in standalone Dart, the callbacks just never run.
   • I need a better way of detecting this case. Some way of telling if the queue is being flushed.
   • This also means that we'll need github CI to run these tests through flutter test as well as the current dart test, since the behavior is different.
   • UPDATE: There's no good way of detecting this, so the best option is probably to add a macro that we only define in the flutter plugin build. But since the flutter plugin is the only really supported way of using package:objective_c, I think I'll just switch the github CI to use flutter test and ignore the standalone embedder. We will need to handle this case when we switch to native assets though.