sentry-dart icon indicating copy to clipboard operation
sentry-dart copied to clipboard

Improve `FileSystemTransport` by using `compute()` or background isolates

Open ueman opened this issue 4 years ago • 8 comments
trafficstars

We can try to use compute() here

https://github.com/getsentry/sentry-dart/blob/bca59abc7f5f257b4fcbb973090d9df719509ebc/flutter/lib/src/file_system_transport.dart#L14-L17

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);
  }
}

ueman avatar Jul 22 '21 08:07 ueman

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.

marandaneto avatar Jan 11 '22 13:01 marandaneto

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

ueman avatar Jan 11 '22 13:01 ueman

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)

bruno-garcia avatar Jan 17 '22 23:01 bruno-garcia

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 avatar Jan 13 '23 13:01 marandaneto

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 avatar Jan 22 '24 13:01 denrase

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 #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?

denrase avatar Jan 30 '24 11:01 denrase

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 avatar Jan 30 '24 13:01 buenaflor

Blocked by min version Flutter 3.7

buenaflor avatar Jan 30 '24 16:01 buenaflor