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

feat: uint8list instead of int list #1010

Closed
wants to merge 2 commits into from

Conversation

evan361425
Copy link

@evan361425 evan361425 commented Oct 6, 2024

Purpose

Using Uint8List to pass the value as proposed in #954.

Why?

To avoid converting between strings and byte lists. There are also several benefits shown in this video.

Tests

I've tested this on Android, and it works as expected, but I haven't tested it on iOS (I don't have a physical device).

Breaking changes

We can easily convert between Uint8List and List<int>:

final v = <int>[1, 2, 3];
print(v.join(',')); // 1,2,3
final vv = Uint8List.fromList(v);
print(vv.join(',')); // 1,2,3
final vvv = vv as List<int>;
print(vvv.join(',')); // 1,2,3

However, this is still a breaking change. How can we make the migration painless? We could provide a value8 in the property and deprecated getter of value8, for example:

class BmDescriptorData {
  final Uint8List value8;

  factory BmDescriptorData.fromMap(Map<dynamic, dynamic> json) {
    return BmDescriptorData(
      value8: json['value'],
      // ...
    );
  }

  // @deprecate
  List<int> get value => value8 as List<int>;
}

But I'm not sure whether we should offer this migration path or simply bump the major version.

@evan361425 evan361425 changed the title feat: uint8list instead of list of int feat: uint8list instead of int list Oct 6, 2024
@@ -2279,7 +2279,7 @@ public void onCharacteristicChanged(BluetoothGatt gatt, BluetoothGattCharacteris
LogLevel level = LogLevel.DEBUG;
log(level, "onCharacteristicChanged:");
log(level, " chr: " + uuidStr(characteristic.getUuid()));
onCharacteristicReceived(gatt, characteristic, value, status);
onCharacteristicReceived(gatt, characteristic, value, BluetoothGatt.GATT_SUCCESS);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug, using status which is undefined.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@chipweinberger
Copy link
Owner

chipweinberger commented Oct 6, 2024

unfortunately, these are breaking changes to the API.

so I wont merge.

its a good idea though.

I wouldnt consider this PR without perf numbers. BLE data rates are extremely slow. like 200KB/s. I think a phone can easily convert Lists at this slow speed.

@evan361425
Copy link
Author

Will it be considered if I make it backward compatible?

@chipweinberger
Copy link
Owner

chipweinberger commented Oct 6, 2024

you would need to measure perf impact at the minimum

only if it was a big impact would i consider it

@evan361425
Copy link
Author

OK, I'll try it but there's still a bug anyway.

@chipweinberger
Copy link
Owner

chipweinberger commented Oct 16, 2024

hi, did you measure the performance impact yet?

Does it make a noticeable difference?

@evan361425
Copy link
Author

Still in plan😂

@evan361425
Copy link
Author

evan361425 commented Nov 10, 2024

@chipweinberger
Thanks for your patience. I tested it in the performance_screen with the following data:

  • Request size: 512 bytes
  • Requests per epoch: 100
  • Epoch count: 100 (resulting in 10,000 requests per test scenario)

My testing device has several properties:

  • Local device served by bumble
  • Write without response
  • Device will ignore the data it received

Here are the results (each value is in seconds):

Scenario Sum Avg P90 P95 P99
List of int to string 83.006 0.83006 0.7985 0.9328 1.0313
Uint8List 79.806 0.79806 0.7945 0.8771 0.8971
Raw data

List of int to string:

1.3480
0.94000
0.96700
1.0560
1.1900
0.97400
1.0770
1.0650
0.93200
0.83600
1.0300
0.88700
0.84800
0.91000
0.86000
0.88900
0.84000
0.86600
0.85200
0.86500
0.80900
0.80800
0.79300
0.80100
0.83800
0.81000
0.82700
0.78600
0.82500
0.90900
0.78900
0.81300
0.82800
0.78900
0.81400
0.81700
0.79700
0.80000
0.82000
0.73900
0.80300
0.78700
0.75400
0.91000
0.80900
0.79500
0.78700
0.80800
0.78500
0.79400
0.78500
0.78100
0.78800
0.78000
0.77900
0.79700
0.72700
0.80100
0.76700
0.78200
0.77100
0.76500
0.77100
0.76700
0.75400
0.73700
0.82400
0.77100
0.72700
0.79400
0.77100
0.78400
0.79200
0.88500
0.98500
0.85000
0.79000
0.79000
0.84300
0.81300
0.75100
0.76900
0.79000
0.85400
0.91100
0.79600
0.76500
0.74300
0.77100
0.85900
0.80100
0.74200
0.78500
0.76900
0.75300
0.76900
0.78000
0.76200
0.80500
0.78400

Uint8List:

1.0850
0.97900
0.89100
0.87700
0.88500
0.84900
0.87400
0.90200
0.89700
0.84000
0.84300
0.85800
0.90000
0.81400
0.81500
0.84600
0.85600
0.85900
0.89900
0.82100
0.81100
0.78300
0.88400
0.85200
0.80300
0.79700
0.83200
0.83200
0.81500
0.78700
0.82600
0.87300
0.82200
0.80900
0.78300
0.76400
0.84000
0.81000
0.81100
0.80400
0.83800
0.78300
0.80800
0.83200
0.80500
0.75900
0.78600
0.77900
0.78800
0.87800
0.79500
0.80600
0.77400
0.77100
0.78700
0.78000
0.76000
0.72600
0.83900
0.79200
0.74400
0.74400
0.82800
0.73500
0.71300
0.74600
0.82300
0.77600
0.77200
0.76000
0.74200
0.79400
0.76000
0.72500
0.75400
0.68900
0.68500
0.81700
0.80100
0.79600
0.78700
0.75500
0.72800
0.68600
0.71400
0.70400
0.80800
0.72200
0.71800
0.77700
0.73400
0.72800
0.72800
0.76700
0.77000
0.76700
0.70500
0.72200
0.73400
0.73400

@chipweinberger
Copy link
Owner

chipweinberger commented Nov 10, 2024

thanks for the numbers!!

a nice little improvement! but not enough of a difference to break api.

we could avoid converting to strings in the internal code, and probably get similar improvements.

but i like hex, it's more useful for during application development, logs, debugging issues people file on github, etc.


there are a lot more important changes we could make to improve perf, first.

for example, right now we block using a mutex and don't queue up any more writes until dart knows the previous write is finished.

but we could have a small queue on the native side, to avoid round tripping through Dart every time.

then instead of the user calling

for(..) {await chr.write(...);}`

they would need to use:

for(..) {chr.write(...);}

but again, would need perf numbers.

@chipweinberger
Copy link
Owner

chipweinberger commented Nov 10, 2024

but we could have a small queue on the native side, to avoid round tripping through Dart every time.

this is an idea i've wanted to try for awhile. it would be annoying to implement. but easy enough to test to see how much it improves things.

test # 1: do lots of writes with FBP using dart
test # 2: do lots of writes using native only (or you could add a new "test" function to FBP)

and compare.

@chipweinberger
Copy link
Owner

chipweinberger commented Nov 10, 2024

we could avoid converting to strings in the internal code, and probably get similar improvements.
but i like hex, it's more useful for during application development, logs, debugging issues people file on github, etc.

thinking about this more, we could still log hex for verbose logs. we just need to update the logging code on the dart side to convert Uint8List to hex strings before we log.

      // log args
      if (logLevel == LogLevel.verbose) {
        String func = '<$method>';
        String args = arguments.toString();
        func = _logColor ? _black(func) : func;
        args = _logColor ? _magenta(args) : args;
        print("[FBP] $func args: $args");
      }

      // invoke
      out = await _methodChannel.invokeMethod(method, arguments);

      // log result
      if (logLevel == LogLevel.verbose) {
        String func = '($method)';
        String result = out.toString();
        func = _logColor ? _black(func) : func;
        result = _logColor ? _brown(result) : result;
        print("[FBP] $func result: $result");
      }

feel free to submit a new PR. switch to Uint8List for internal comms, and just use hex when logging. youll probably still get 99% of the perf improvement you're seeing.

@evan361425
Copy link
Author

I'll try, thanks.

@evan361425
Copy link
Author

evan361425 commented Nov 10, 2024

By the way, the result above could serve as a benchmark for future improvements, although only through the performance_screen can we compare against this benchmark.

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.

2 participants