rmawatson / flutter_isolate

Launch an isolate that can use flutter plugins.
MIT License
262 stars 80 forks source link

[Feature] iOS: use FlutterEngineGroup for better perf #151

Open chipweinberger opened 7 months ago

chipweinberger commented 7 months ago

As the title says, use FlutterEngineGroup for better perf on iOS.

There is a separate PR for android: https://github.com/rmawatson/flutter_isolate/pull/150

this closes https://github.com/rmawatson/flutter_isolate/issues/149 https://github.com/rmawatson/flutter_isolate/issues/111 https://github.com/rmawatson/flutter_isolate/issues/73

I've tested these changes on iOS 17.2.1 and Flutter 3.16.8

About me, I am the maintainer of these packages, so I am pretty familiar with flutter plugins

nmfisher commented 7 months ago

The example project is crashing for me on iOS 16.6.1 with the following:

2024-01-31 08:32:32.216820+0800 Runner[93603:12522867] *** Assertion failure in -[FlutterDownloaderPlugin startBackgroundIsolate:], FlutterDownloaderPlugin.m:145
2024-01-31 08:32:32.218135+0800 Runner[93603:12522867] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'failed to set registerPlugins'
*** First throw call stack:
(0x193f48cb4 0x18cfe83d0 0x18e6de54c 0x1009ab92c 0x1009b1014 0x1009b42fc 0x103ce89b8 0x103755718 0x100b68520 0x100b6a038 0x100b7a798 0x100b7a2dc 0x193fd7c28 0x193fb9560 0x193fbe3ec 0x1cf4d435c 0x19634af58 0x19634abbc 0x1006adf08 0x1b34f0dec)
libc++abi: terminating due to uncaught exception of type NSException

Same happens with #152.

chipweinberger commented 7 months ago

I had a lot of problems with flutter_downloader in general.

Does it crash without my PR?

And does it work without flutter_downloader?

Thanks for helping. I did not hit this crash so I can't really debug it.

I'll run it again right now though to confirm.

chipweinberger commented 7 months ago

this is the code that is failing

https://github.com/fluttercommunity/flutter_downloader/blob/master/ios/Classes/FlutterDownloaderPlugin.m

- (void)startBackgroundIsolate:(int64_t)handle {
    if (debug) {
        NSLog(@"startBackgroundIsolate");
    }
    FlutterCallbackInformation *info = [FlutterCallbackCache lookupCallbackInformation:handle];
    NSAssert(info != nil, @"failed to find callback");
    NSString *entrypoint = info.callbackName;
    NSString *uri = info.callbackLibraryPath;
    [_headlessRunner runWithEntrypoint:entrypoint libraryURI:uri];
    NSAssert(registerPlugins != nil, @"failed to set registerPlugins");

    // Once our headless runner has been started, we need to register the application's plugins
    // with the runner in order for them to work on the background isolate. `registerPlugins` is
    // a callback set from AppDelegate.m in the main application. This callback should register
    // all relevant plugins (excluding those which require UI).
    registerPlugins(_headlessRunner);
    [_registrar addMethodCallDelegate:self channel:_callbackChannel];
}
nmfisher commented 7 months ago

I had a lot of problems with flutter_downloader in general.

Does it crash without my PR?

And does it work without flutter_downloader?

Thanks for helping. I did not hit this crash so I can't really debug it.

I'll run it again right now though to confirm.

Yes, the example project runs fine for me on cdf8ef31bfc909ba546f7edb3a6f6a1ae6b38df2.

flutter_downloader is a bit flaky but the fact it worked before makes me think we accidentally broke something here :)

chipweinberger commented 7 months ago

yes I can reproduce the crash on 1.17.2 actually. Not sure how I missed that, since I'm pretty sure I tested it...

chipweinberger commented 7 months ago

tbh, I'm not familiar enough with this stuff to debug it. maybe you are?

If we can't fix it, you could still merge my PRs and then revert this commit if you like. the macOS support is nice!

Like others, I use this plugin to allow multiple dart ui instances. If we can't fix this, I will probably make a simpler plugin called dart_ui_isolate just for the dart:ui use case. Without FlutterEngineGroup, my code crashes due to too much memory use. So no choice!

nmfisher commented 7 months ago

Actually that's precisely the question I was going to ask - given Flutter now natively supports running plugins in background isolates, the only remaining use case for this package is running dart:ui functions in a background isolate. That's why this package is mostly in maintenance mode.

I think it makes sense to create a dart_ui_isolate package for that use case (i.e. using the code in your PRs), mark this package as deprecated and point users who need dart:ui support to that new package. Thoughts?

chipweinberger commented 7 months ago

ya that makes sense to me.

I started work on dart_ui_isolate.

chipweinberger commented 7 months ago

https://pub.dev/packages/dart_ui_isolate

chipweinberger commented 7 months ago

theres actually still a use case for flutter_isolate

you might want to call both dart:ui code and platform plugin code in your isolate.

I don't need this. But someone might.

nmfisher commented 7 months ago

Fair point. If anyone has that issue, I'd lean towards telling them to use a vanilla isolate for plugin code, and dart_ui_isolate for their dart:ui code, and shuffle data between the two if they need. Maintaining this package for the (presumably) tiny number of people who need that is quite a burden.

Or if they don't like that, then they can submit a PR to fix whatever broke :)