capacitor-community / bluetooth-le

Capacitor plugin for Bluetooth Low Energy
MIT License
284 stars 86 forks source link

perf: byte to hex string conversion optimizations #701

Closed NaterGator closed 3 weeks ago

NaterGator commented 1 month ago

Per #698 I found the performance of the plugin to be a problem when moving data through a notification characteristic as fast as possible.

This PR is focused on that use-case; high-throughput data transfer from peripheral to the central. There may be additional performance gains to squeeze from the reverse path, however I haven't tested that case yet and suspect it is not as significant of an issue.

I would appreciate extra scrutiny on the optimized Swift conversion function, as I could only test using the windows swift toolchain on my PC. The TypeScript side passes the existing conversion spec tests and the Kotlin side has been tested running on my Pixel 8 Pro passing data from an nRF devkit (the nature of this specific app requires the egressed data to contain a SHA-256 hash which is checked by the central so I am very confident data integrity is being maintained there).

I ran a profiling session moving 512KB of data from this implementation and recorded a wall time of only 124.61ms to perform the byte->hexstring conversion: image

Appreciative of any feedback or suggested improvements necessary to land this. 👍

peitschie commented 1 month ago

Nice work @NaterGator

I'll do some testing on this later this week hopefully. It looks like the lint target has failed... are you able to fix this up?

NaterGator commented 1 month ago

@peitschie updated to fix the linting error and failing tests. Unfortunately I can't run verify:ios or lint:ios on win32, so I'll need to wait for the CI build to see if the fallback iOS<14 path passes.

NaterGator commented 3 weeks ago

I just pushed some demo projects that might help with evaluating these changes.

There's a Zephyr project to pump a BLE notification characteristic with as much data as possible available here: https://github.com/NaterGator/zephyr-ble-notif-throughput I've included hex builds for the Nordic nRF52832 and nRF52840 Development Kits.

There's a Vue 3 capacitor app here: https://github.com/NaterGator/capacitor-ble-indication-throughput

When running together you get something like this which makes for a pretty easy profiling target... Screenshot_20241026-100634

peitschie commented 3 weeks ago

I've tested this on Android and iOS and can confirm the conversion all works as expected on both platforms. Thanks for the careful work here @NaterGator ... your harnesses and associate embedded code helped speed things up a lot!

@pwespi is there anything further you'd like to see here before we squash and merge?

pwespi commented 3 weeks ago

I've tested this on Android and iOS and can confirm the conversion all works as expected on both platforms. Thanks for the careful work here @NaterGator ... your harnesses and associate embedded code helped speed things up a lot!

@pwespi is there anything further you'd like to see here before we squash and merge?

Looks good to me. Thank you for the great contribution @NaterGator!

pwespi commented 3 weeks ago

@all-contributors please add @NaterGator for code

allcontributors[bot] commented 3 weeks ago

@pwespi

I've put up a pull request to add @NaterGator! :tada: