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

perf: byte to hex string conversion optimizations #701

Merged
merged 12 commits into from
Oct 28, 2024

Conversation

NaterGator
Copy link
Contributor

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. 👍

Benchmarks can be found at https://www.measurethat.net/Benchmarks/Show/32317/0/parse-hex-to-bytes---test-capacitor-bluetooth-le-conver

Performance improvement is platform dependent but appears to be about 9x faster on iOS and Android
In the future the Kotlin Stdlib may support this conversion, but it is currently marked as experimental. This implementation is less configurable but perhaps a bit faster.

Passing about 512KB of data through a notification characteristic with the old method utilized about 21.5 seconds of wall clock time. This implementation utilized  124.61 milliseconds, a whopping 172X speedup. When using a notification characteristic to transfer binary data to the central over BLE, this speedup is very noticeable.
Unfortunately, I don't have a Mac to build an iOS test case, so I ran some toy benchmarking on my PC using the Swift 6.0.1 toolchain for Windows.

I ran 1000 iterations of converting a 4KB Data blob to a hex string using the existing algorithm, which took 25.14 seconds to finish. Running the same test with this proposed implementation took 0.91 seconds, yielding a speedup of "only" ~27x. While this isn't as significant as the Kotlin optimization, this is my first time writing any Swift code, so there may be additional performance improvements to explore. On that front, I would appreciate scrutiny (and testing, if possible!) from iOS developers. I ran some test cases on Windows, and everything looked fine.
@peitschie
Copy link
Collaborator

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?

Benchmarking this implementation on my PC shows that it runs at about half the speed of the fast path but is still substantially faster than the existing implementation:

iOS14+Conv: Time taken for 1000 iterations with data size 4096 bytes: 0.8684617 seconds
FallbackConv: Time taken for 1000 iterations with data size 4096 bytes: 1.3471951 seconds
BaselineConv: Time taken for 1000 iterations with data size 4096 bytes: 23.8809594 seconds
@NaterGator
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Collaborator

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 pwespi changed the title Byte to hex string conversion optimizations perf: byte to hex string conversion optimizations Oct 28, 2024
@pwespi
Copy link
Member

pwespi commented Oct 28, 2024

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!

@peitschie peitschie merged commit e064587 into capacitor-community:main Oct 28, 2024
4 checks passed
@pwespi
Copy link
Member

pwespi commented Oct 30, 2024

@all-contributors please add @NaterGator for code

Copy link
Contributor

@pwespi

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

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.

3 participants