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

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

Open
NaterGator opened this issue Oct 19, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@NaterGator
Copy link

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 NaterGator added the enhancement New feature or request label Oct 19, 2024
@NaterGator
Copy link
Author

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants