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

Improve FileSystemTransport by using compute() or background isolates #536

Open
ueman opened this issue Jul 22, 2021 · 8 comments
Open

Improve FileSystemTransport by using compute() or background isolates #536

ueman opened this issue Jul 22, 2021 · 8 comments

Comments

@ueman
Copy link
Collaborator

ueman commented Jul 22, 2021

We can try to use compute() here

final envelopeData = <int>[];
await envelope.envelopeStream().forEach(envelopeData.addAll);
// https://flutter.dev/docs/development/platform-integration/platform-channels#codec
final args = [Uint8List.fromList(envelopeData)];

in order to offload the conversion to another isolate.
This could improve the performance because we're not blocking the UI Isolate, especially if there are any attachments or big events.

There are some things which need to be considered.

class NewFileSystemTransport implements Transport {
  NewFileSystemTransport(this._channel, this._options);

  final MethodChannel _channel;
  final SentryOptions _options;

  @override
  Future<SentryId?> send(SentryEnvelope envelope) async {
    final args = await compute(_convert, envelope);
    try {
      await _channel.invokeMethod<void>('captureEnvelope', [args]);
    } catch (exception, stackTrace) {
      _options.logger(
        SentryLevel.error,
        'Failed to save envelope',
        exception: exception,
        stackTrace: stackTrace,
      );
      return SentryId.empty();
    }

    return envelope.header.eventId;
  }

  static Future<Uint8List> _convert(SentryEnvelope envelope) async {
    final envelopeData = <int>[];
    await envelope.envelopeStream().forEach(envelopeData.addAll);
    // https://flutter.dev/docs/development/platform-integration/platform-channels#codec
    return Uint8List.fromList(envelopeData);
  }
}
@marandaneto
Copy link
Contributor

makes sense, since Flutter Apps don't crash if not on the Native layer, we could offload the heavy work on a different Isolate using compute, worth saying that every capture method returns a Future, so they can always choose to not await the method anyway.

@ueman
Copy link
Collaborator Author

ueman commented Jan 11, 2022

Even if it's not awaited it's still running on the UI isolate.

@bruno-garcia
Copy link
Member

if we're not awaiting, we need another back pressure mechanism. Otherwise we might degrade performance if the app is producing more events than the SDK can flush out (to disk or network)

@marandaneto
Copy link
Contributor

marandaneto commented Jan 13, 2023

Background isolates might help solving this issue, if we are able to offload to a background Isolate and this Isolate is able to call method channels, only possible on Flutter 3.7 still.

Edit:
3.7 is GA
See https://medium.com/flutter/whats-new-in-flutter-3-7-38cbea71133c

@marandaneto marandaneto changed the title Improve FileSystemTransport by using compute() Improve FileSystemTransport by using compute() or background isolates Jan 25, 2023
@denrase
Copy link
Collaborator

denrase commented Jan 22, 2024

Checked the documentation for background channel usage, and we have the same Issue that we are facing with #1801, as we need to access ServicesBinding.rootIsolateToken, which is static, so we can't use the approach where we cast to dynamic and call a method that is unknown in older Flutter versions which we need to support. Is there approach or a way around this limitation?

@denrase
Copy link
Collaborator

denrase commented Jan 30, 2024

Short summary from the #1811 findings:

  • We can't use compute to convert the envelope, as we capture objects that cannot be shared between isolates. Same issues as in Enhancement: Use compute to offload heavy work from the main thread #707
  • We'd need to bump flutter to 3.7 in order to use method channels in a background isolate. Is passing the serialised structures to the Cocoa/Android SDKs on the main thread a bottleneck that is causing us issues?

@buenaflor
Copy link
Contributor

Is passing the serialised structures to the Cocoa/Android SDKs on the main thread a bottleneck that is causing us issues?

Have you checked that?

Not sure if passing serialized data will cause bottleneck issues so I'd think this is just a nice to have in terms of improvement but alone probably doesn't warrant a min version bump to 3.7

@buenaflor
Copy link
Contributor

Blocked by min version Flutter 3.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Blocked
8 participants