[Bluetooth, Performance] Cache the Bluetooth adapter name on Mac.

[Bluetooth, Performance] Cache the Bluetooth adapter name on Mac.

BUG=461181
R=erikchen@chromium.org, scheib@chromium.org

Committed: https://crrev.com/35d9c4606318947a2711b760085f66d174a30b31
Cr-Commit-Position: refs/heads/master@{#327423}

[Ilya Sherman, 2015-04-22 01:07:05]

Note: I haven't actually tested this change, because I'm not super familiar with
the requisite steps.  It seems straightforward enough, though.  Are y'all okay
with this very naïve caching solution?

[erikchen, 2015-04-22 01:29:55]

It's not clear to me that this solution is correct, especially if you
haven't been able to test.

We don't know what's causing the slowness. Perhaps there is an
initialization cost that was being paid in -nameAsString, which will now be
paid in addressAsString. Why do we need to poll with this high frequency to
begin with? Can this name property in fact change? Can the
defaultController change?

I think that reducing the poll frequency, or even calling this code on
demand when it's needed would be a more reasonable solution.

On Tue, Apr 21, 2015 at 6:07 PM, <isherman@chromium.org> wrote:

> Reviewers: erikchen, scheib,
>
> Message:
> Note: I haven't actually tested this change, because I'm not super
> familiar with
> the requisite steps.  It seems straightforward enough, though.  Are y'all
> okay
> with this very naïve caching solution?
>
> Description:
> [Bluetooth, Performance] Cache the Bluetooth adapter name on Mac.
>
> BUG=461181
> R=erikchen@chromium.org, scheib@chromium.org
>
> Please review this at https://codereview.chromium.org/1096383006/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+9, -3 lines):
>   M device/bluetooth/bluetooth_adapter_mac.mm
>
>
> Index: device/bluetooth/bluetooth_adapter_mac.mm
> diff --git a/device/bluetooth/bluetooth_adapter_mac.mm b/device/bluetooth/
> bluetooth_adapter_mac.mm
> index
>
7c04a433c419cdf6d69e6adf0d12b2065bb26d6d..63bf1b8f59684a0cda9c998f9c4fd584cdb33d65
> 100644
> --- a/device/bluetooth/bluetooth_adapter_mac.mm
> +++ b/device/bluetooth/bluetooth_adapter_mac.mm
> @@ -268,7 +268,6 @@ void BluetoothAdapterMac::PollAdapter() {
>        FROM_HERE_WITH_EXPLICIT_FUNCTION(
>            "461181 BluetoothAdapterMac::PollAdapter::Start"));
>    bool was_present = IsPresent();
> -  std::string name;
>    std::string address;
>    bool powered = false;
>    IOBluetoothHostController* controller =
> @@ -280,14 +279,21 @@ void BluetoothAdapterMac::PollAdapter() {
>        FROM_HERE_WITH_EXPLICIT_FUNCTION(
>            "461181 BluetoothAdapterMac::PollAdapter::GetControllerStats"));
>    if (controller != nil) {
> -    name = base::SysNSStringToUTF8([controller nameAsString]);
>      address = BluetoothDevice::CanonicalizeAddress(
>          base::SysNSStringToUTF8([controller addressAsString]));
>      powered = ([controller powerState] == kBluetoothHCIPowerStateON);
> +
> +    // For performance reasons, cache the adapter's name. It's not
> uncommon for
> +    // a call to [controller nameAsString] to take tens of milliseconds.
> Note
> +    // that this caching strategy might result in clients receiving a
> stale
> +    // name. If this is a significant issue, then some more sophisticated
> +    // workaround for the performance bottleneck will be needed. For
> additional
> +    // context, see http://crbug.com/461181 and http://crbug.com/467316
> +    if (name_.is_empty())
> +      name_ = base::SysNSStringToUTF8([controller nameAsString]);
>    }
>
>    bool is_present = !address.empty();
> -  name_ = name;
>    address_ = address;
>
>    // TODO(erikchen): Remove ScopedTracker below once
> http://crbug.com/461181
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2015-04-22 01:30:17]

Maybe we should be holding on to a reference of the defaultController?

On Tue, Apr 21, 2015 at 6:29 PM, Erik Chen <erikchen@chromium.org> wrote:

> It's not clear to me that this solution is correct, especially if you
> haven't been able to test.
>
> We don't know what's causing the slowness. Perhaps there is an
> initialization cost that was being paid in -nameAsString, which will now be
> paid in addressAsString. Why do we need to poll with this high frequency to
> begin with? Can this name property in fact change? Can the
> defaultController change?
>
> I think that reducing the poll frequency, or even calling this code on
> demand when it's needed would be a more reasonable solution.
>
> On Tue, Apr 21, 2015 at 6:07 PM, <isherman@chromium.org> wrote:
>
>> Reviewers: erikchen, scheib,
>>
>> Message:
>> Note: I haven't actually tested this change, because I'm not super
>> familiar with
>> the requisite steps.  It seems straightforward enough, though.  Are y'all
>> okay
>> with this very naïve caching solution?
>>
>> Description:
>> [Bluetooth, Performance] Cache the Bluetooth adapter name on Mac.
>>
>> BUG=461181
>> R=erikchen@chromium.org, scheib@chromium.org
>>
>> Please review this at https://codereview.chromium.org/1096383006/
>>
>> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>>
>> Affected files (+9, -3 lines):
>>   M device/bluetooth/bluetooth_adapter_mac.mm
>>
>>
>> Index: device/bluetooth/bluetooth_adapter_mac.mm
>> diff --git a/device/bluetooth/bluetooth_adapter_mac.mm
>> b/device/bluetooth/bluetooth_adapter_mac.mm
>> index
>>
7c04a433c419cdf6d69e6adf0d12b2065bb26d6d..63bf1b8f59684a0cda9c998f9c4fd584cdb33d65
>> 100644
>> --- a/device/bluetooth/bluetooth_adapter_mac.mm
>> +++ b/device/bluetooth/bluetooth_adapter_mac.mm
>> @@ -268,7 +268,6 @@ void BluetoothAdapterMac::PollAdapter() {
>>        FROM_HERE_WITH_EXPLICIT_FUNCTION(
>>            "461181 BluetoothAdapterMac::PollAdapter::Start"));
>>    bool was_present = IsPresent();
>> -  std::string name;
>>    std::string address;
>>    bool powered = false;
>>    IOBluetoothHostController* controller =
>> @@ -280,14 +279,21 @@ void BluetoothAdapterMac::PollAdapter() {
>>        FROM_HERE_WITH_EXPLICIT_FUNCTION(
>>            "461181
>> BluetoothAdapterMac::PollAdapter::GetControllerStats"));
>>    if (controller != nil) {
>> -    name = base::SysNSStringToUTF8([controller nameAsString]);
>>      address = BluetoothDevice::CanonicalizeAddress(
>>          base::SysNSStringToUTF8([controller addressAsString]));
>>      powered = ([controller powerState] == kBluetoothHCIPowerStateON);
>> +
>> +    // For performance reasons, cache the adapter's name. It's not
>> uncommon for
>> +    // a call to [controller nameAsString] to take tens of milliseconds.
>> Note
>> +    // that this caching strategy might result in clients receiving a
>> stale
>> +    // name. If this is a significant issue, then some more sophisticated
>> +    // workaround for the performance bottleneck will be needed. For
>> additional
>> +    // context, see http://crbug.com/461181 and http://crbug.com/467316
>> +    if (name_.is_empty())
>> +      name_ = base::SysNSStringToUTF8([controller nameAsString]);
>>    }
>>
>>    bool is_present = !address.empty();
>> -  name_ = name;
>>    address_ = address;
>>
>>    // TODO(erikchen): Remove ScopedTracker below once
>> http://crbug.com/461181
>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2015-04-28 04:01:19]

I've now locally verified the results that jyasskin observed here [1], and that
this patch removes the bottleneck, rather than merely shifting it.  So, we
return to my earlier question: Is this form of caching an acceptable solution? 
Note that users can change their Bluetooth adapter's name via System Preferences
~> Sharing ~> Computer Name.

[1] https://code.google.com/p/chromium/issues/detail?id=467316#c13

[Ilya Sherman, 2015-04-28 04:12:08]

On 2015/04/28 04:01:19, Ilya Sherman wrote:
> I've now locally verified the results that jyasskin observed here [1], and
that
> this patch removes the bottleneck, rather than merely shifting it.  So, we
> return to my earlier question: Is this form of caching an acceptable solution?

> Note that users can change their Bluetooth adapter's name via System
Preferences
> ~> Sharing ~> Computer Name.
> 
> [1] https://code.google.com/p/chromium/issues/detail?id=467316#c13

Also, please note that as described in the linked comment, [controller
addressAsString] also blocks the UI thread for a couple of milliseconds.  It's
not as egregious, but still not great.  I wonder if this polling can, in fact,
be moved to a background thread, given that it's only reading data.  My only
concern is that I seem to recall reading that the Bluetooth APIs are not
threadsafe, though I don't recall anymore where I read this, and I'm not sure
what exactly that would mean for read-only properties like these.  Erik, do you
have any advice for where to look for more information about this?

[scheib, 2015-04-28 04:37:54]

On 2015/04/28 04:12:08, Ilya Sherman wrote:
> On 2015/04/28 04:01:19, Ilya Sherman wrote:
> > I've now locally verified the results that jyasskin observed here [1], and
> that
> > this patch removes the bottleneck, rather than merely shifting it.  So, we
> > return to my earlier question: Is this form of caching an acceptable
solution?
> 
> > Note that users can change their Bluetooth adapter's name via System
> Preferences
> > ~> Sharing ~> Computer Name.
> > 
> > [1] https://code.google.com/p/chromium/issues/detail?id=467316#c13
> 
> Also, please note that as described in the linked comment, [controller
> addressAsString] also blocks the UI thread for a couple of milliseconds.  It's
> not as egregious, but still not great.  I wonder if this polling can, in fact,
> be moved to a background thread, given that it's only reading data.  My only
> concern is that I seem to recall reading that the Bluetooth APIs are not
> threadsafe, though I don't recall anymore where I read this, and I'm not sure
> what exactly that would mean for read-only properties like these.  Erik, do
you
> have any advice for where to look for more information about this?

Caching seems better than the continuous performance issues, though as you
observe still an issue. 2ms is still pretty large on the UI thread, I don't
think we can let that remain either. I couldn't find threading issues when
searching -- it seems tempting to me to try using a worker thread.

[scheib, 2015-04-28 04:38:41]

On 2015/04/28 04:37:54, scheib wrote:
> On 2015/04/28 04:12:08, Ilya Sherman wrote:
> > On 2015/04/28 04:01:19, Ilya Sherman wrote:
> > > I've now locally verified the results that jyasskin observed here [1], and
> > that
> > > this patch removes the bottleneck, rather than merely shifting it.  So, we
> > > return to my earlier question: Is this form of caching an acceptable
> solution?
> > 
> > > Note that users can change their Bluetooth adapter's name via System
> > Preferences
> > > ~> Sharing ~> Computer Name.
> > > 
> > > [1] https://code.google.com/p/chromium/issues/detail?id=467316#c13
> > 
> > Also, please note that as described in the linked comment, [controller
> > addressAsString] also blocks the UI thread for a couple of milliseconds. 
It's
> > not as egregious, but still not great.  I wonder if this polling can, in
fact,
> > be moved to a background thread, given that it's only reading data.  My only
> > concern is that I seem to recall reading that the Bluetooth APIs are not
> > threadsafe, though I don't recall anymore where I read this, and I'm not
sure
> > what exactly that would mean for read-only properties like these.  Erik, do
> you
> > have any advice for where to look for more information about this?
> 
> Caching seems better than the continuous performance issues, though as you
> observe still an issue. 2ms is still pretty large on the UI thread, I don't
> think we can let that remain either. I couldn't find threading issues when
> searching -- it seems tempting to me to try using a worker thread.

Also I just filed a bug https://bugreport.apple.com on the API. Unfortunately
you can't share bug references, Ilya you may wish to file one also.

[erikchen, 2015-04-28 17:50:27]

On 2015/04/28 04:38:41, scheib wrote:
> On 2015/04/28 04:37:54, scheib wrote:
> > On 2015/04/28 04:12:08, Ilya Sherman wrote:
> > > On 2015/04/28 04:01:19, Ilya Sherman wrote:
> > > > I've now locally verified the results that jyasskin observed here [1],
and
> > > that
> > > > this patch removes the bottleneck, rather than merely shifting it.  So,
we
> > > > return to my earlier question: Is this form of caching an acceptable
> > solution?
> > > 
> > > > Note that users can change their Bluetooth adapter's name via System
> > > Preferences
> > > > ~> Sharing ~> Computer Name.
> > > > 
> > > > [1] https://code.google.com/p/chromium/issues/detail?id=467316#c13
> > > 
> > > Also, please note that as described in the linked comment, [controller
> > > addressAsString] also blocks the UI thread for a couple of milliseconds. 
> It's
> > > not as egregious, but still not great.  I wonder if this polling can, in
> fact,
> > > be moved to a background thread, given that it's only reading data.  My
only
> > > concern is that I seem to recall reading that the Bluetooth APIs are not
> > > threadsafe, though I don't recall anymore where I read this, and I'm not
> sure
> > > what exactly that would mean for read-only properties like these.  Erik,
do
> > you
> > > have any advice for where to look for more information about this?
> > 
> > Caching seems better than the continuous performance issues, though as you
> > observe still an issue. 2ms is still pretty large on the UI thread, I don't
> > think we can let that remain either. I couldn't find threading issues when
> > searching -- it seems tempting to me to try using a worker thread.
> 
> Also I just filed a bug https://bugreport.apple.com on the API. Unfortunately
> you can't share bug references, Ilya you may wish to file one also.

IOBluetooth must be used from the main thread:
http://lists.apple.com/archives/bluetooth-dev/2004/Jul/msg00027.html

Given the severity of the jank caused by -[controller nameAsString], I'm going
to lgtm this change. There will be problems with devices that have multiple
bluetooth adapters (which should be rare), or when the user changes the name of
the adapter (which should also be rare), but we can deal with those if they
become significant user concerns.

[scheib, 2015-04-28 17:55:39]

https://codereview.chromium.org/1096383006/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1096383006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:276: if (name_.empty())
" || address_ != address" will at least catch adapter changes.

[Ilya Sherman, 2015-04-28 23:42:26]

isherman@chromium.org changed reviewers:
+ armansito@chromium.org

[Ilya Sherman, 2015-04-28 23:42:26]

+Arman for OWNERS approval.

https://codereview.chromium.org/1096383006/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1096383006/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:276: if (name_.empty())
On 2015/04/28 17:55:39, scheib wrote:
> " || address_ != address" will at least catch adapter changes.

Good idea.  Done.

[armansito, 2015-04-28 23:51:47]

lgtm

[Ilya Sherman, 2015-04-28 23:59:13]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-04-28 23:59:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1096383006/#ps60001
(title: "Also refresh name if the BT address changes")

[commit-bot: I haz the power, 2015-04-29 00:00:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096383006/60001

[commit-bot: I haz the power, 2015-04-29 02:35:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-29 02:36:00]

Patchset 4 (id:??) landed as
https://crrev.com/35d9c4606318947a2711b760085f66d174a30b31
Cr-Commit-Position: refs/heads/master@{#327423}