from jdoerrie: I personally observed very slow repeated calls to WriteRemoteCharacteristic, which might be one of the reasons.

Hmm interesting. Does the peripheral's characteristics support write without response?

Even then, I can't think of a reason as to why mac would be slower in this regard. When we write to a characteristic that supports writes without response, we immediately post a task to call the success callback. That has to be faster than waiting for BlueZ to reply over DBUS...

I did some more investigation, here is a log dump of what is happening:
http://gpaste/4997613803798528 (apologies for internal link, the important bits are copied below). As you can see it takes 11 seconds in total.

What stands out to me is that round-trips through the OS take half a second for write operations, e.g. like this:

[19741:775:0406/180150.330444:VERBOSE1:bluetooth_remote_gatt_characteristic_mac.mm(199)] [...] : Write characteristic.
[19741:775:0406/180150.825308:VERBOSE1:bluetooth_remote_gatt_characteristic_mac.mm(329)] [...]: Write value succeeded.

Re #c2: It looks like the default is to WriteWithReponse, as evident from here:
 https://codesearch.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm?l=422-427&rcl=400c1e296933da71f3b4e3a04b0843297dd4ffa4

I'm not sure whether this is a bug, or intended behavior.

The repeated write calls can be sped up by returning CBCharacteristicWriteWithoutResponse from that method, but the service discovery etc is still slow. 

For example, it takes 1.5 seconds to start a discovery session:
[19741:775:0406/180147.360915:VERBOSE1:bluetooth_remote_gatt_characteristic_mac.mm(220)] [...]: Subscribe to characteristic.
[19741:775:0406/180148.845500:VERBOSE1:bluetooth_remote_gatt_characteristic_mac.mm(173)] [...]: Read characteristic.

Pretty much exactly half a second later the response to the read request arrives:
[19741:775:0406/180149.340566:VERBOSE1:bluetooth_remote_gatt_characteristic_mac.mm(269)] [...]: Read request arrived.


Would you happen to know what could cause this delay, ortuno@? My first instinct was the Adapter Polling Interval of 500ms [1], but this is unrelated. Changing the interval did not speed up the operation.

Just for reference, this is how Linux performs. Everything is done within 3 seconds: http://gpaste/5545150092673024

[1] https://codesearch.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_mac.mm?l=52&rcl=4a5c0e6db24bbdfad1f374018ab9fe5bd23dd1ea

Any chance we can change the device characteristic properties to only support write without response? That way that method would return write without response. Another option is to add a new method. I don't think it would make sense to add the option to write without response to the current method since it takes a callback. Preferably we would change the device.

Regarding the delay for discovery, I think that might be normal because we are discovering all attributes. As a last resort we could introduce more low level methods to retrieve specific attributes and not do the full discovery.

I ran some benchmarks to compare the performance of WriteWithResponse and WriteWithoutResponse. To do so, I logged the transfer rates of sending and receiving 1KB split into chunks of 20 bytes. Screenshots of the results are attached. As you can see, writing without response increased the Data sent rate from ~30 Bytes/Sec to ~250 Bytes/Sec, an 8x improvement.

This improvement reduces the overall time needed for the GetAssertion operation from 11 seconds to 7.5 seconds.

I'll send out a CL that introduces WriteRemoteCharacteristicWithoutResponse with a fall back to WriteRemoteCharacteristic. 

Deleted: Send_and_Receive_1KB_With_Response.png 159 KB

	Send_and_Receive_1KB_With_Response.png 159 KB View Download

Deleted: Send_and_Receive_1KB_Without_Response.png 158 KB

	Send_and_Receive_1KB_Without_Response.png 158 KB View Download

Before you go that route, another alternative is to negotiate a bigger MTU.
That might allow us to send more data for each round trip.

Unfortunately, this is not possible in our case, the Security Key demands a MTU of 20 bytes (see `fidoControlPointLength` in [1]). Also I don't think setting the MTU is possible on all platforms.

Re alternatives: The easiest fix is of course to replace 

CBCharacteristicWriteType BluetoothRemoteGattCharacteristicMac::GetCBWriteType()
    const {
  return (GetProperties() & BluetoothGattCharacteristic::PROPERTY_WRITE)
             ? CBCharacteristicWriteWithResponse
             : CBCharacteristicWriteWithoutResponse;
}

with

CBCharacteristicWriteType BluetoothRemoteGattCharacteristicMac::GetCBWriteType()
    const {
  return (GetProperties() & BluetoothGattCharacteristic::PROPERTY_WRITE_WITHOUT_RESPONSE)
             ? CBCharacteristicWriteWithoutResponse
             : CBCharacteristicWriteWithResponse;
}

but this changes the semantics quite a bit. Would you be happy with such as change? I do have a first draft of WriteRemoteCharacteristicWithoutResponse in https://crrev.com/c/1000859.

[1] https://fidoalliance.org/specs/fido-v2.0-rd-20170927/fido-client-to-authenticator-protocol-v2.0-rd-20170927.html#fido-service

For context, can you help me understand the consequences of making this change?

Ahh too bad about MTU.

There are basically two types of writes one can perform on a characteristic. Write and WriteWithoutResponse. In the former, the central (in this case Chrome) waits for the peripheral to reply with success or failure. In the later, the peripheral is not expected to reply.

Most devices only specify one of these methods, but there are some that, for one reason or another, specify both even though they don't support both. For example, earlier Eddystone beacons used to indicate that they supported both of these methods, but when trying to use write without response they would fail to save the new settings. On the other side, some devices that indicate they support both methods, sometimes fail to send a response when using the regular write method.

From an API perspective
- Android uses WriteWithoutResponse by default but can be specified
- BlueZ uses Write by default but cannot be specified
- macOS uses Write by default but can be specified
- Windows uses Write by default but can be specified
- Unsure what Cast does

Based on the fact that we already have some discrepancy here, I wouldn't be opposed to changing the default, as long as we change it everywhere possible. We are replacing BlueZ, so we could fix that one in the future.

Related: Issue 672648

Would it be feasible to enable clients to choose whether they want writes with or without response? At least on macOS the only way to trigger a WriteWithoutResponse is having the remote characteristic only signal the WriteWithoutResponse property. However, given that `properties` is read-only on the CBCharacteristic [1], there is no way for Chrome to actually trigger that behavior if the Characteristic also supports a regular Write.

I think it would be nice to allow clients to choose the type of writes they want to do if both writing with and without a response is possible (one example use case is this very issue).

I could think of two ways:

1) Introduce WriteRemoteCharacteristicWithoutResponse on BluetoothRemoteGattCharacteristic. This will attempt to do a write without response, and only fall back to a write with response if the characteristic does not support it (i.e. current behavior of Android). We will keep WriteRemoteCharacteristic with the reversed behavior, i.e. attempt a write with response, and fall back to a write without response if writing with response is not supported (i.e. the current behavior on Mac and Windows).

2) Instead of introducing a new WriteRemoteCharacteristicWithoutResponse method we add an enum argument to WriteRemoteCharacteristic that specifies the preferred response type. We could make this argument optional, and consider either one of the options as the default.

WDYT?

[1] https://developer.apple.com/documentation/corebluetooth/cbcharacteristic/1519010-properties?language=objc

I think the new method makes sense, but I would prefer if we didn't fall back to a regular write. My main issue with Write is that it's callback based which doesn't make much sense for WriteWithoutResponse. I think it would be easier implementation-wise if WriteWithoutResponse was:

bool BluetoothRemoteGattCharacteristic::WriteWithoutReponse(std::vector<uint8_t> value);

We have some hacks on macOS to handle WriteWithoutResponse, because macOS doesn't fire an event for it, but it does for a regular write.

That said, I'm not too attached to that approach and don't see any problems with 2) (except that we still have a callback for a write without response :p) as longs as it's clearly documented.

Ack, makes sense. I actually considered 

bool BluetoothRemoteGattCharacteristic::WriteWithoutReponse(std::vector<uint8_t> value);

but then had troubles falling back to WriteRemoteCharacteristic in case writing without response was not supported. Not having the fallback definitively simplifies things. 

Furthermore, it is probably not too much to ask for client code to handle the fallback themselves if desired, e.g. like this:

  std::vector<uint8_t> data = ...;
  if (!characteristic->WriteWithoutResponse(data)) {
    characteristic->WriteRemoteCharacteristic(data, callback, error_callback).
  }

I'll update https://crrev.com/c/1000859 to this effect.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7786fe58397c2087caee44e80e3ac36bbee49b4

commit a7786fe58397c2087caee44e80e3ac36bbee49b4
Author: Jan Wilken Dörrie <jdoerrie@chromium.org>
Date: Thu Apr 12 12:31:05 2018

[bluetooth] Introduce WriteWithoutResponse()

This change introduces WriteWithoutResponse() on
BluetoothRemoteGattCharacteristic. In contrast to the regular
WriteRemoteCharacteristic() this method does not wait for a response
from the operating system. For now it is only implemented on Mac, where
it can result in a speedup of 8x for sending data
(see  https://crbug.com/808175#c9 ).

Bug:  808175 , 831524
Change-Id: If2012ac8e19d17fe218e273d8d0e300a2b06a19a
Reviewed-on: https://chromium-review.googlesource.com/1000859
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Conley Owens <cco3@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550157}
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/bluetooth_remote_gatt_characteristic.h
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/test/fake_remote_gatt_characteristic.cc
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/bluetooth/test/fake_remote_gatt_characteristic.h
[modify] https://crrev.com/a7786fe58397c2087caee44e80e3ac36bbee49b4/device/fido/fido_ble_connection.cc

Just a quick update:

I updated to macOS High Sierra 10.13.4 this morning and observed faster MakeCredential and GetAssertion operations, in particular there is no longer a half-second delay for OS level round trips. It now takes only about 2 seconds from starting to initiate a GATT connection to fully receiving the device response, internal paste here [1].

I then also measured the transfer rates again, with the results attached. Test set-up was sending 10 x 1KB ping messages with an MTU of 20 bytes (ping messages instruct the device to simply create a response with the request payload). Results in words:

When doing unconfirmed writes read and write speed was around 1.7 KB/s, another ~7x improvement compared to #c9. For confirmed writes the write speed dropped by ~50% (i.e. 850 Bytes/s), which should be expected.

Judging by the latest commits to //device/bluetooth there haven't been any recent significant changes to Chrome's macOS Bluetooth implementation. This is why I suspect the 10.13.4 update to be responsible, even though the changelog does not mention BLE [2].

Since the performance is great for both my Mac devices, I'm marking this bug as fixed now. However, it would be good if someone else is able to reproduce my findings, so it could even be marked Verified.

[1] http://gpaste/4685036754305024
[2] https://support.apple.com/en-us/HT208533

Deleted: Ping_10x_1KB_Confirmed_Writes.png 320 KB

	Ping_10x_1KB_Confirmed_Writes.png 320 KB View Download

Deleted: Ping_10x_1KB_Unconfirmed_Writes.png 166 KB

	Ping_10x_1KB_Unconfirmed_Writes.png 166 KB View Download

That's awesome to hear! What previous version of macOS were you on? Maybe there are more changes in-between?

I am not certain, but I believe I was on 10.13.3 before. I usually install system updates quite quickly. I might still have 10.13.3 at home, and can try to see which performance I'm getting there.