Skip to content

Conversation

@marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Jan 18, 2022

📜 Description

Enhancement: Use compute to offload heavy work from the main thread

💡 Motivation and Context

Closes #536

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Jan 18, 2022

Fails
🚫 Please consider adding a changelog entry for the next release.
`Instructions and example for changelog`$

Please add an entry to CHANGELOG.md` to the "Unreleased" section under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Use compute to offload heavy work from the main thread(#707)

If none of the above apply, you can opt out by adding _#skip-changelog_ to the PR description.

Generated by 🚫 dangerJS against 7dc24d2

@ueman
Copy link
Collaborator

ueman commented Jan 18, 2022

Does it work for file attachments?

@marandaneto
Copy link
Contributor Author

getting the error:

[sentry] [error] Error while capturing exception
[sentry] ArgumentError (Invalid argument(s): Illegal argument in isolate message: (object is aReceivePort))
[sentry] #0 Isolate._spawnFunction (dart:isolate-patch/isolate_patch.dart:395:25)
#1 Isolate.spawn (dart:isolate-patch/isolate_patch.dart:375:7)
#2 compute
package:flutter/…/foundation/_isolates_io.dart:22
#3 FileSystemTransport.send
package:sentry_flutter/src/file_system_transport.dart:16

@marandaneto
Copy link
Contributor Author

marandaneto commented Jan 18, 2022

Does it work for file attachments?

Didn't test it yet, not working even for Events right now.
I dare to say its related to the limited data types accepted by https://api.dart.dev/stable/2.14.4/dart-isolate/SendPort-class.html

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2022

Codecov Report

Merging #707 (7dc24d2) into main (f84275e) will increase coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
+ Coverage   90.67%   90.96%   +0.28%     
==========================================
  Files         104        8      -96     
  Lines        3326      155    -3171     
==========================================
- Hits         3016      141    -2875     
+ Misses        310       14     -296     
Impacted Files Coverage Δ
flutter/lib/src/file_system_transport.dart
dart/lib/src/protocol/contexts.dart
dart/lib/src/protocol/sentry_transaction.dart
dart/lib/src/sentry_tracer_finish_status.dart
dart/lib/src/sentry_envelope.dart
dart/lib/src/version.dart
dart/lib/src/noop_hub.dart
dart/lib/src/scope.dart
dart/lib/src/protocol/breadcrumb.dart
dart/lib/src/http_client/sentry_http_client.dart
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f84275e...7dc24d2. Read the comment docs.

debugLabel: 'captureEnvelope $eventIdLabel',
);
try {
await _channel.invokeMethod<void>('captureEnvelope', args);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args previously was an array

Suggested change
await _channel.invokeMethod<void>('captureEnvelope', args);
await _channel.invokeMethod<void>('captureEnvelope', [args]);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a comment which tells you which type should get passed. Probably also a test which asserts that 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's indeed another problem, but the error is actually with the compute flag, See #707 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, at least the tests are now passing.

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.

Improve FileSystemTransport by using compute() or background isolates

4 participants