capacitor-community / bluetooth-le

Capacitor plugin for Bluetooth Low Energy
MIT License
285 stars 85 forks source link

Hex<->String conversion performance is pretty bad, some suggested improvements #698

Closed NaterGator closed 4 weeks ago

NaterGator commented 1 month ago

Is your feature request related to a problem? Please describe. Some of my use-cases involve pushing data through a notification characteristic at the highest rates I can manage. I'll admit I am currently doing most of my testing on Android, but I'm finding that the conversion routines to push data through JNI are not great and are a source of bottlenecking. For example, pushing about half a megabyte of data through a notification characteristic currently spends a whopping 22 seconds of wall time in bytesToString. When the implementation is replaced with the experimental kotlin stdlib API toHexString wall time spent in this function drops to a paltry 1.8 seconds for half a megabyte of data transferred.

Similarly, the hexStringToDataView implementation on the WebView side is quite inefficient. When tested in an iOS WkWebView and an Android WebView the current implementation (bluetooth-le) is around 10x slower than the fastest (non regex). I can confirm, replacing the implementation with the non regex version substantially improves response time of the GATT callback.

Describe the solution you'd like I think there are some pretty low hanging performance optimizations to be made to the conversion functions that wrangle data for the JNI layer. I realize a high-throughput use case is probably not what most consumers of this plugin are after, but it's a win-win for everyone if there's a net gain in efficiency.

Describe alternatives you've considered Please see above; I've tested higher performance options on the Kotlin side and the JS side, both of which contribute to dramatic speedups.

Additional context Here's a view of the current implementation performance on Android moving ~0.5MB of data over a notification characteristic: image 21.5 seconds of execution time

Here's a view of the Kotlin StdLib toHexString implementation moving the same amount of data: image 1.8 seconds of execution time

NaterGator commented 1 month ago

The JS implementation is as simple as:

function hexStringToDataView(hex) {
    var bin = [], i, c, isEmpty = 1, buffer;
    for(i = 0; i < hex.length; i++){
        c = hex.charCodeAt(i);
        if(c > 47 && c < 58 || c > 64 && c < 71 || c > 96 && c < 103){
            buffer = buffer << 4 ^ (c > 64 ? c + 9 : c) & 15;
            if(isEmpty ^= 1){
                bin.push(buffer & 0xff);
            }
        }
    }
    return numbersToDataView(bin);
}

The Kotlin implementation is as simple as:

@OptIn(ExperimentalStdlibApi::class)
fun bytesToString(bytes: ByteArray): String {
    val hexFormat = HexFormat { bytes { byteSeparator = " "; upperCase = true } }
    return bytes.toHexString(hexFormat)
}

I'm mostly ingressing data from a BLE device's notification characteristic, not trying to write data to a BLE device as quickly as possible, but I suspect there may be similar inefficiencies in the conversion helpers for the opposite direction as well.

peitschie commented 1 month ago

Hi @NaterGator

Those do look pretty reasonable to me. Do you have any awareness of the stability of the experimental Stdlib part of kotlin?

We'd definitely welcome a patch with this in it. Looks like there's no compatibility issues to worry about, so just a nice clean performance improvement overall.

NaterGator commented 1 month ago

Do you have any awareness of the stability of the experimental Stdlib part of kotlin?

That's a great question. I am primarily a firmware developer so I can't claim authority in Kotlin, but their documentation sounds like it's not appropriate for use in a public library like this: https://kotlinlang.org/docs/components-stability.html#stability-levels-explained

I'll work on benchmarking a native Kotlin implementation that avoids this experimental API until it lands in a more stable fashion. The Java String.format performance seems to be pretty atrocious so even a fairly naive native Kotlin implementation will probably significantly outperform it.

Anyways thanks for your willingness to consider this! I'll work on a PR and some test coverage to ensure this is compatible with the existing implementation and welcome any iOS testing since I don't own Mac hardware to test that side of things.

peitschie commented 4 weeks ago

Thanks for the patch @NaterGator !