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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chipweinberger
Copy link
Contributor

@chipweinberger chipweinberger commented Jan 18, 2024

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

There is a separate PR for android: #150

this closes
#149
#111
#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

@chipweinberger chipweinberger changed the title [Feature] use FlutterEngineGroup on iOS [Feature] iOS: use FlutterEngineGroup for better perf Jan 18, 2024
@nmfisher
Copy link
Collaborator

nmfisher commented Jan 31, 2024

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
Copy link
Contributor Author

chipweinberger commented Jan 31, 2024

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
Copy link
Contributor Author

chipweinberger commented Jan 31, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

chipweinberger commented Jan 31, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

ya that makes sense to me.

I started work on dart_ui_isolate.

@chipweinberger
Copy link
Contributor Author

@chipweinberger
Copy link
Contributor Author

chipweinberger commented Jan 31, 2024

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
Copy link
Collaborator

nmfisher commented Feb 1, 2024

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 :)

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

Successfully merging this pull request may close these issues.

2 participants