refactor: use JNI for Android integration by vaind · Pull Request #2670 · getsentry/sentry-dart

vaind · 2025-02-07T14:58:18Z

📜 Description

This is a first step towards migrating our Android plugin glue code to JNI (part of #1444).

Done in this PR:

added JNI and JNIGEN, currently pinning to a specific version because package:jni is in a nonstable release cycle (0.x) so there may be breaking changes even between minor releases.
use JNI for replay screenshot capture to improve performance
offload the native call & bitmap creation to another isolate

🔮 Next steps

Integration tests for replay capture (Add Flutter E2E & Integration Test #2345)
incrementally migrate more of the android plugin to JNI

💡 Motivation and Context

💚 How did you test it?

Manually & udpated tests

📝 Checklist

I reviewed submitted code
I added tests to verify changes
No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
I updated the docs if needed
All tests passing
No breaking changes

github-actions · 2025-02-10T13:02:33Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1256.73 ms	1266.82 ms	10.08 ms
Size	8.42 MiB	9.91 MiB	1.49 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`a22e451`	1248.37 ms	1270.55 ms	22.19 ms
`88e4bfd`	1236.44 ms	1256.33 ms	19.90 ms
`0db91cc`	1267.63 ms	1279.69 ms	12.06 ms
`90a08ea`	1260.45 ms	1285.24 ms	24.80 ms
`8de999e`	1267.51 ms	1281.00 ms	13.49 ms
`ba9c106`	1241.76 ms	1265.15 ms	23.40 ms
`05933ac`	1258.37 ms	1285.57 ms	27.21 ms
`3334ac1`	1259.22 ms	1275.40 ms	16.17 ms
`3a16179`	1238.18 ms	1255.62 ms	17.44 ms
`c15867f`	1247.36 ms	1264.63 ms	17.26 ms

App size

Revision	Plain	With Sentry	Diff
`a22e451`	8.42 MiB	9.89 MiB	1.47 MiB
`88e4bfd`	8.33 MiB	9.64 MiB	1.31 MiB
`0db91cc`	8.15 MiB	9.15 MiB	1018.56 KiB
`90a08ea`	8.38 MiB	9.73 MiB	1.36 MiB
`8de999e`	8.42 MiB	9.88 MiB	1.46 MiB
`ba9c106`	8.32 MiB	9.38 MiB	1.06 MiB
`05933ac`	8.38 MiB	9.78 MiB	1.40 MiB
`3334ac1`	8.10 MiB	9.17 MiB	1.08 MiB
`3a16179`	8.38 MiB	9.73 MiB	1.35 MiB
`c15867f`	8.42 MiB	9.91 MiB	1.49 MiB

github-actions · 2025-02-10T14:10:21Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt
flutter/lib/src/native/java/android_replay_recorder.dart
flutter/lib/src/screenshot/recorder.dart

codecov · 2025-02-10T14:10:38Z

Codecov Report

Attention: Patch coverage is 4.05405% with 71 lines in your changes missing coverage. Please review.

Project coverage is 88.38%. Comparing base (79ccefb) to head (d97ad48).
Report is 235 commits behind head on v9.

Files with missing lines	Patch %	Lines
...r/lib/src/native/java/android_replay_recorder.dart	1.38%	71 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##               v9    #2670       +/-   ##
===========================================
+ Coverage   67.94%   88.38%   +20.43%     
===========================================
  Files          15      261      +246     
  Lines         443     8890     +8447     
===========================================
+ Hits          301     7857     +7556     
- Misses        142     1033      +891

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vaind · 2025-02-12T07:52:44Z

@HosseinYousefi do you mind taking a look?

HosseinYousefi

I checked the JNI related parts and overall LGTM with one small comment.

flutter/lib/src/native/java/android_replay_recorder.dart

HosseinYousefi · 2025-02-12T10:35:14Z

flutter/lib/src/native/java/android_replay_recorder.dart

+    // Android Bitmap creation is a bit costly so we reuse it between captures.
+    native.Bitmap? bitmap;
+
+    final _nativeReplay = native.SentryFlutterPlugin$Companion(null)


Please open an issue for JNIgen to remove the synthetic "default constructor" marker added by the Kotlin compiler from the generated code and just pass null.

HosseinYousefi · 2025-02-12T10:50:01Z

flutter/lib/src/native/java/android_replay_recorder.dart

+        // https://developer.android.com/reference/android/graphics/Bitmap#createBitmap(int,%20int,%20android.graphics.Bitmap.Config)
+        // Note: in the currently generated API this may return null so we null-check below.
+        bitmap ??= native.Bitmap.createBitmap$3(
+            item.width, item.height, native.Bitmap$Config.ARGB_8888);


.ARGB_8888 creates a references which is not eagerly released.

Thanks for pointing it out, I've actually had a suspicion that may be the case but it doesn't matter in this case as it's only executed once for each orientation change so it's fine if it's collected by GC

HosseinYousefi · 2025-02-12T10:51:42Z

flutter/lib/src/native/java/android_replay_recorder.dart

+import 'binding.dart' as native;

+// Note, this is currently not unit-tested because mocking the JNI calls is
+// cumbersome, see https://github.com/dart-lang/native/issues/1794


Please let me know what your ideal mocking solution would look like in the issue.

HosseinYousefi · 2025-02-12T10:55:25Z

flutter/microbenchmarks/lib/src/jni_bench.dart

+import 'package:benchmarking/benchmarking.dart';
+import 'package:flutter/foundation.dart';
+
+Future<void> execute() async {


Out of curiousity, have you measured the perfomance improvement between the previous way of using method channels vs. using jni?

Not per se. I have profiled it previously and with the new code and the biggest benefit we get is that we can now use Bitmap directly from raw RGBA so we don't have to convert to PNG (which is in hundreds of milliseconds, even though it's async) and to write an intermediary file (two IO ops - write & read). So there really is no one-to-one comparison between the approaches.
Another benefit was being able to move the work to another isolate altogether. Actually, I need to check whether I can pass an Uint8List efficiently between isolates. Last time I checked it didn't work.

buenaflor

can't comment much on the jni parts but lgtm so far

CHANGELOG.md

Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>

vaind added 11 commits February 6, 2025 21:17

add JNI and use in replay.onScreenshotRecorded

03ea861

exlude bindings from coverage

8719ce1

use jni bitmap

2270862

move all native to android_replay_recorder

a87e80f

android screenshot isolate

b20afac

cleanup

c7acf2d

use persistent isolate

f17c72d

update replay native tests

0517a36

update high-risk-code list

069c726

remove print()

d986a26

remove package:file dependency

f804774

vaind added 2 commits February 10, 2025 14:48

JNI benchmark

3b054bd

Merge branch 'v9' into refactor/jni

d23f781

vaind changed the base branch from main to v9 February 10, 2025 14:14

vaind added 11 commits February 10, 2025 17:05

ktlint format

6995e12

fixup ci

09202be

fixup

8094735

fix web tests

2f5fd6e

dart format

f6aee8a

try to fix ci

baeb5f6

try to fix ci

c1a2f56

chore: exclude binding from coverage

3308015

comments

db4e700

Merge branch 'v9' into refactor/jni

b28071b

chore: changelog

1ba949f

vaind marked this pull request as ready for review February 12, 2025 07:51

vaind requested a review from krystofwoldrich as a code owner February 12, 2025 07:51

vaind requested review from buenaflor and stefanosiano as code owners February 12, 2025 07:51

HosseinYousefi reviewed Feb 12, 2025

View reviewed changes

move uint8list creation to separate isolate

0506e8c

buenaflor approved these changes Feb 12, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

vaind and others added 2 commits February 12, 2025 22:26

Update CHANGELOG.md

96c0458

Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>

Merge branch 'v9' into refactor/jni

d97ad48

vaind merged commit e1f690d into v9 Feb 13, 2025
145 of 147 checks passed

vaind deleted the refactor/jni branch February 13, 2025 14:28

vaind mentioned this pull request Feb 18, 2025

refactor: cleanup platform mocking #2730

Merged

6 tasks

RCVZ mentioned this pull request Feb 26, 2025

Performance improvement session reply PostHog/posthog-flutter#165

Open

Uh oh!

Conversation

vaind commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

Done in this PR:

🔮 Next steps

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

Uh oh!

github-actions bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Uh oh!

github-actions bot commented Feb 10, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vaind commented Feb 12, 2025

Uh oh!

HosseinYousefi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HosseinYousefi Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

HosseinYousefi Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

vaind Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HosseinYousefi Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

HosseinYousefi Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

vaind Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

buenaflor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vaind commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

vaind Feb 12, 2025 •

edited

Loading