Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1261593004:
bluetooth: Add histograms and logging for requestDevice() (Closed)
|
Created:
5 years, 4 months ago by ortuno Modified:
5 years, 4 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-adapter-ff Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Add histograms and logging for requestDevice()
Four new histograms:
- RequestDevice.Filters.Count: Number of filters used when scanning
- RequestDevice.Filters.Services: Services used when scanning
- RequestDevice.OptionalServices.Count: Number of optionalServices passed.
- RequestDevice.OptionalServices.Services: Optional services
- RequestDevice.UnionOfServices.Count: Number of services used.
- RequestDevice.FilterSize: Number of services in a filter.
One Rename:
Bluetooth.RequestDevice.Outcome -> Bluetooth.Web.RequestDevice.Outcome
Logging:
- VLOG(1) filters used when scanning
- VLOG(1) optional services used when scanning
- VLOG(1) any errors before starting scan
- VLOG(1) devices found and their UUIDs
- VLOG(1) if no devices were found
BUG=499636
Committed: https://crrev.com/f61dcf839472db5b0a8a3dc0f241929f17ddb8ff
Cr-Commit-Position: refs/heads/master@{#343139}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Add union of services and # of filters. Remove services list. #
Total comments: 12
Patch Set 3 : Remove unnecessary VLOGs #Patch Set 4 : Address asvitkine comments. #
Total comments: 17
Patch Set 5 : Use hash of UUID #
Total comments: 2
Patch Set 6 : Change macro #
Total comments: 2
Patch Set 7 : Use histogram_macros.h instead #
Total comments: 11
Patch Set 8 : Address jyasskin's comments #
Total comments: 5
Patch Set 9 : Remove old enum. Fix typoes #Patch Set 10 : Remove unnecessary include #
Total comments: 4
Patch Set 11 : Address scheib's comments #Patch Set 12 : Use SuperFastHash directly. #
Dependent Patchsets: Messages
Total messages: 48 (13 generated)
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: PTAL https://codereview.chromium.org/1261593004/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:417: VLOG(1) << "requestDevice called with the following filters: "; Not sure about this one. If someone is having problems it would be useful to know what filters they are using and chromeos doesn't log filters.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
@jyasskin: ping?
https://codereview.chromium.org/1261593004/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/80001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:107: std::map<BluetoothUUID, UMAGATTServices>* getServiceToEnumMap() { I don't really like having to maintain yet another map of standardized UUIDs, or the code size needed to make it work just for a histogram we don't have concrete plans for. If we need this, we should compute the histogrammed value algorithmically from the UUID's contents. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2724: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Count" units="filters"> I'm not sure we should include this. It's only 1 dimension of the filters argument, and I don't have any picture of how we'd improve the API or implementation just knowing how many or'ed alternatives people are passing in. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2735: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Services" This might be helpful in future API development, but I don't think it'll help the Web Bluetooth API. Even to help with future development, I think we're going to want to watch more than the standardized services, and histograms aren't really a good tool for a 128-bit space. If we do use this, all standardized services so far have UUIDs of the form 000018xy-0000-1000-8000-00805f9b34fb. Let's just pass the xy to the enumeration? Maybe use 0xFF for the "unknown" value. We'll need to re-do it when they allocate more than 255 services, but that'll take a while. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2747: +<histogram name="Bluetooth.Web.RequestDevice.OptionalServices.Count" Again, what do we learn from the count of optional services? At least this is the whole length of the argument, but I'm having trouble thinking of what we'll use it for.
ortuno@chromium.org changed reviewers: + scheib@chromium.org
https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2724: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Count" units="filters"> On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > I'm not sure we should include this. It's only 1 dimension of the filters argument, and I don't have any picture of how we'd improve the API or implementation just knowing how many or'ed alternatives people are passing in. If we plan on informing the users the services that a website is trying to access then this data could help with the UI decisions: Knowing how many filters are normally used, how many services are normally used when scanning, etc. could be used the same way the number of devices was used to decide how big the BT device chooser is on ChromeOS. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2735: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Services" On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > This might be helpful in future API development, but I don't think it'll help the Web Bluetooth API. Even to help with future development, I think we're going to want to watch more than the standardized services, and histograms aren't really a good tool for a 128-bit space. > > If we do use this, all standardized services so far have UUIDs of the form 000018xy-0000-1000-8000-00805f9b34fb. Let's just pass the xy to the enumeration? Maybe use 0xFF for the "unknown" value. We'll need to re-do it when they allocate more than 255 services, but that'll take a while. OK. So instead of having the enums we would just have a count histogram? https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2747: +<histogram name="Bluetooth.Web.RequestDevice.OptionalServices.Count" On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > Again, what do we learn from the count of optional services? At least this is the whole length of the argument, but I'm having trouble thinking of what we'll use it for. I think this would give us a pretty good idea of what's the Advertised/Non-advertised service ratio. Similarly knowing which services are used here would tell us which services are advertised and which aren't.
https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2724: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Count" units="filters"> On 2015/08/05 22:05:12, ortuno wrote: > On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > > I'm not sure we should include this. It's only 1 dimension of the filters > argument, and I don't have any picture of how we'd improve the API or > implementation just knowing how many or'ed alternatives people are passing in. > > If we plan on informing the users the services that a website is trying to > access then this data could help with the UI decisions: Knowing how many filters > are normally used, how many services are normally used when scanning, etc. could > be used the same way the number of devices was used to decide how big the BT > device chooser is on ChromeOS. For that, we need the total number of services, including both filters and optional services. We don't yet compute that, but we'll need to in order to store the allowed services set. Let's do that. There's a start in ComputeScanFilter(). This would be named something like "Bluetooth.Web.RequestDevice.AllowedServices.Count", right? https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2735: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Services" On 2015/08/05 22:05:12, ortuno wrote: > On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > > This might be helpful in future API development, but I don't think it'll help > the Web Bluetooth API. Even to help with future development, I think we're going > to want to watch more than the standardized services, and histograms aren't > really a good tool for a 128-bit space. > > > > If we do use this, all standardized services so far have UUIDs of the form > 000018xy-0000-1000-8000-00805f9b34fb. Let's just pass the xy to the enumeration? > Maybe use 0xFF for the "unknown" value. We'll need to re-do it when they > allocate more than 255 services, but that'll take a while. > > OK. So instead of having the enums we would just have a count histogram? I'd prefer to just have the AllowedServices.Count histogram until we have a specific use in mind for recording standardized services. One specific use might be in checking whether people use a lot of non-standard services. I believe they'll use more non-standard than standard ones, but I could be wrong. For that, we need a 2-bit bitset: has-standard-services (defined as having 16-bit UUIDs?) and has-non-standard-services. I could see recording this for both the filters and the optional_services. When looking at higher-level APIs to standardize, I think we'll be better off looking at npm and bower for popular libraries, than just looking at which standardized services are used. Once we have some candidates, we could add a more targeted histogram to decide between them, but I don't think we should just record everything up front. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2747: +<histogram name="Bluetooth.Web.RequestDevice.OptionalServices.Count" On 2015/08/05 22:05:12, ortuno wrote: > On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > > Again, what do we learn from the count of optional services? At least this is > the whole length of the argument, but I'm having trouble thinking of what we'll > use it for. > > I think this would give us a pretty good idea of what's the > Advertised/Non-advertised service ratio. Similarly knowing which services are > used here would tell us which services are advertised and which aren't. If we want to know what fraction of services devices tend to advertise, we should collect that in the discovery code, not from requestDevice() calls.
Patchset #2 (id:100001) has been deleted
ortuno@chromium.org changed reviewers: + asvitkine@chromium.org
@asvitkine: Could you take a look at histograms.xml? https://codereview.chromium.org/1261593004/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/80001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:107: std::map<BluetoothUUID, UMAGATTServices>* getServiceToEnumMap() { On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > I don't really like having to maintain yet another map of standardized UUIDs, or the code size needed to make it work just for a histogram we don't have concrete plans for. If we need this, we should compute the histogrammed value algorithmically from the UUID's contents. Done. https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2724: +<histogram name="Bluetooth.Web.RequestDevice.Filters.Count" units="filters"> On 2015/08/05 at 22:55:41, Jeffrey Yasskin wrote: > On 2015/08/05 22:05:12, ortuno wrote: > > On 2015/08/05 at 19:03:49, Jeffrey Yasskin wrote: > > > I'm not sure we should include this. It's only 1 dimension of the filters > > argument, and I don't have any picture of how we'd improve the API or > > implementation just knowing how many or'ed alternatives people are passing in. > > > > If we plan on informing the users the services that a website is trying to > > access then this data could help with the UI decisions: Knowing how many filters > > are normally used, how many services are normally used when scanning, etc. could > > be used the same way the number of devices was used to decide how big the BT > > device chooser is on ChromeOS. > > For that, we need the total number of services, including both filters and optional services. We don't yet compute that, but we'll need to in order to store the allowed services set. Let's do that. There's a start in ComputeScanFilter(). This would be named something like "Bluetooth.Web.RequestDevice.AllowedServices.Count", right? I don't see the harm in having a breakdown of the allowed services and the number of filters. I can imagine a general purpose ble website scanning for each service in a different filter. We surely want to express that differently than all services in a single filter.
https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; Nit: const char kBaseStandardServiceUUID[] = https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb Nit: {}'s https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return serviceEnum + 1; nit: hacker_style https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: GetServiceEnum(service), 0xFF); Why is the max value 0xFF? https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const std::set<BluetoothUUID> unionOfServices(optional_services.begin(), Nit: camel_case https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << "requestDevice called with the following filters: "; VLOGs are generally discouraged (search chromium-dev), consider using DVLOG instead of removing entirely from the CL.
On 2015/08/10 at 20:12:45, asvitkine wrote: > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; > Nit: const char kBaseStandardServiceUUID[] = > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb > Nit: {}'s > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return serviceEnum + 1; > nit: hacker_style > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: GetServiceEnum(service), 0xFF); > Why is the max value 0xFF? > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const std::set<BluetoothUUID> unionOfServices(optional_services.begin(), > Nit: camel_case > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << "requestDevice called with the following filters: "; > VLOGs are generally discouraged (search chromium-dev), consider using DVLOG instead of removing entirely from the CL. Still addressing the issues. But I have a question regarding VLOGs. We have seen some reports about people not finding bluetooth devices and it would be very useful to know what devices the platform is seeing. What would you recommend instead of using VLOG?
Do histograms provide enough information? If so, you can ask them to provide you the output of chrome://histograms/Bluetooth.Web On Mon, Aug 10, 2015 at 4:16 PM, <ortuno@chromium.org> wrote: > On 2015/08/10 at 20:12:45, asvitkine wrote: > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* >> > kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; > >> Nit: const char kBaseStandardServiceUUID[] = >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, >> > kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb > >> Nit: {}'s >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return >> serviceEnum >> > + 1; > >> nit: hacker_style >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: >> > GetServiceEnum(service), 0xFF); > >> Why is the max value 0xFF? >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const >> > std::set<BluetoothUUID> unionOfServices(optional_services.begin(), > >> Nit: camel_case >> > > > > https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... > >> content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << >> > "requestDevice called with the following filters: "; > >> VLOGs are generally discouraged (search chromium-dev), consider using >> DVLOG >> > instead of removing entirely from the CL. > > Still addressing the issues. But I have a question regarding VLOGs. We > have seen > some reports about people not finding bluetooth devices and it would be > very > useful to know what devices the platform is seeing. What would you > recommend > instead of using VLOG? > > https://codereview.chromium.org/1261593004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
They do not. We want to know specific information about the devices the underlying platform is returning to chrome. Information that doesn't get histogrammed. On Mon, Aug 10, 2015 at 1:19 PM Alexei Svitkine <asvitkine@chromium.org> wrote: > Do histograms provide enough information? If so, you can ask them to > provide you the output of chrome://histograms/Bluetooth.Web > > On Mon, Aug 10, 2015 at 4:16 PM, <ortuno@chromium.org> wrote: > >> On 2015/08/10 at 20:12:45, asvitkine wrote: >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* >>> >> kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; >> >>> Nit: const char kBaseStandardServiceUUID[] = >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, >>> >> kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb >> >>> Nit: {}'s >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return >>> serviceEnum >>> >> + 1; >> >>> nit: hacker_style >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: >>> >> GetServiceEnum(service), 0xFF); >> >>> Why is the max value 0xFF? >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const >>> >> std::set<BluetoothUUID> unionOfServices(optional_services.begin(), >> >>> Nit: camel_case >>> >> >> >> >> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >> >>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << >>> >> "requestDevice called with the following filters: "; >> >>> VLOGs are generally discouraged (search chromium-dev), consider using >>> DVLOG >>> >> instead of removing entirely from the CL. >> >> Still addressing the issues. But I have a question regarding VLOGs. We >> have seen >> some reports about people not finding bluetooth devices and it would be >> very >> useful to know what devices the platform is seeing. What would you >> recommend >> instead of using VLOG? >> >> https://codereview.chromium.org/1261593004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I guess vlog is ok to use then. If it's something that's a common use case, you can also consider adding chrome://bluetooth-internals page. On Mon, Aug 10, 2015 at 4:21 PM, Giovanni Ortuño <ortuno@google.com> wrote: > They do not. We want to know specific information about the devices the > underlying platform is returning to chrome. Information that doesn't get > histogrammed. > > On Mon, Aug 10, 2015 at 1:19 PM Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Do histograms provide enough information? If so, you can ask them to >> provide you the output of chrome://histograms/Bluetooth.Web >> >> On Mon, Aug 10, 2015 at 4:16 PM, <ortuno@chromium.org> wrote: >> >>> On 2015/08/10 at 20:12:45, asvitkine wrote: >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* >>>> >>> kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; >>> >>>> Nit: const char kBaseStandardServiceUUID[] = >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, >>>> >>> kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb >>> >>>> Nit: {}'s >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return >>>> serviceEnum >>>> >>> + 1; >>> >>>> nit: hacker_style >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: >>>> >>> GetServiceEnum(service), 0xFF); >>> >>>> Why is the max value 0xFF? >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const >>>> >>> std::set<BluetoothUUID> unionOfServices(optional_services.begin(), >>> >>>> Nit: camel_case >>>> >>> >>> >>> >>> https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... >>> >>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << >>>> >>> "requestDevice called with the following filters: "; >>> >>>> VLOGs are generally discouraged (search chromium-dev), consider using >>>> DVLOG >>>> >>> instead of removing entirely from the CL. >>> >>> Still addressing the issues. But I have a question regarding VLOGs. We >>> have seen >>> some reports about people not finding bluetooth devices and it would be >>> very >>> useful to know what devices the platform is seeing. What would you >>> recommend >>> instead of using VLOG? >>> >>> https://codereview.chromium.org/1261593004/ >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@asvitkine: PTAL https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:35: const char* kBaseStandardServiceUUID = "00001800-0000-1000-8000-00805f9b34fb"; On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > Nit: const char kBaseStandardServiceUUID[] = Done. https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, kBaseStandardServiceUUID)) // -0000-1000-8000-00805f9b34fb On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return serviceEnum + 1; On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > nit: hacker_style Done? https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:113: GetServiceEnum(service), 0xFF); On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > Why is the max value 0xFF? Changed it to 0x101. Standard Service UUIDs are between 0x1800 and 0x18FF but we only histogram the last two digits and since we shift everything by 1 the max value becomes 0x101. https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:120: const std::set<BluetoothUUID> unionOfServices(optional_services.begin(), On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > Nit: camel_case Done. https://codereview.chromium.org/1261593004/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:341: VLOG(1) << "requestDevice called with the following filters: "; On 2015/08/10 at 20:12:45, Alexei Svitkine wrote: > VLOGs are generally discouraged (search chromium-dev), consider using DVLOG instead of removing entirely from the CL. I read the email thread about VLOG. Since we are actively monitoring and debugging problems with Web Bluetooth we are keeping the VLOGs. Eventually we will add a about:bluetooth-devices page, but we suspect it might have a bigger binary size impact than VLOGs. See: http://crbug.com/519010
I am ok with using vlogs here for the reasons you mention. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:82: service_enum <<= 4; Instead of doing this complicated logic, have you considered just logging the hash of service.canonical_value() in an enumerated histogram> https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:83: service_enum += base::HexDigitToInt(service.canonical_value()[i + 6]); What guarantees that i+6 is in range? https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return ++service_enum; This should be service_enum + 1, since incrementing the local doesn't do anything here. https://codereview.chromium.org/1261593004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1261593004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2722: -<histogram name="Bluetooth.RequestDevice.Outcome" It's better to mark the existing histogram <obsolete> so that old data can still be seen on the dashboard. Unless no data has been collected for this yet.
https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:38: const uint16_t kMaxStandardServiceUUID = 0x22; Let's not force ourselves to update this every time they come up with a new service UUID. Just pass the full 2 hexdigits? However, a constant for the 0x101 would be useful. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:73: if (!service.canonical_value().compare(0, 6, Please use ==0 for .compare(). It's a tri-valued function, not a boolean, and ! tends to be confusing. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, kBaseStandardServiceUUID)) { // -0000-1000-8000-00805f9b34fb This comparison isn't quite right. kBaseStandardServiceUUID will be compared from the beginning, but you want to compare from kBaseStandardServiceUUID + 8. Can we have a unittest for this? https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:82: service_enum <<= 4; On 2015/08/10 22:03:35, Alexei Svitkine wrote: > Instead of doing this complicated logic, have you considered just logging the > hash of service.canonical_value() in an enumerated histogram> We didn't consider that, but 1) what's the maximum value for that enumeration? 2) The BT standards body gives us a contiguous range of values; it seems odd to spread them out and risk collisions by hashing. However, we don't need a loop here. const std::string& uuid = service.canonical_value(); uint16_t service_enum = base::HexDigitToInt(uuid[6]) * 0x10 + base::HexDigitToInt(uuid[7]); should be enough. We could CHECK_EQ(36, uuid.size()) if that makes anyone more comfortable. On the other hand, if we can easily send a 32-bit hash of the 128-bit UUID, that'll make it easier to check for the popularity of non-standard services, which would be better. How do we do that with the histogram macros? They seem to always want a maximum+1 that fits in an int, and they allocate 'maximum' ints, which is a lot of memory. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:83: service_enum += base::HexDigitToInt(service.canonical_value()[i + 6]); On 2015/08/10 22:03:36, Alexei Svitkine wrote: > What guarantees that i+6 is in range? https://code.google.com/p/chromium/codesearch/#chromium/src/device/bluetooth/..., but it'd make sense to CHECK.
https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:82: service_enum <<= 4; On 2015/08/10 22:18:56, Jeffrey Yasskin wrote: > On 2015/08/10 22:03:35, Alexei Svitkine wrote: > > Instead of doing this complicated logic, have you considered just logging the > > hash of service.canonical_value() in an enumerated histogram> > > We didn't consider that, but 1) what's the maximum value for that enumeration? > 2) The BT standards body gives us a contiguous range of values; it seems odd to > spread them out and risk collisions by hashing. > > However, we don't need a loop here. > > const std::string& uuid = service.canonical_value(); > uint16_t service_enum = base::HexDigitToInt(uuid[6]) * 0x10 + > base::HexDigitToInt(uuid[7]); > > should be enough. We could CHECK_EQ(36, uuid.size()) if that makes anyone more > comfortable. > > On the other hand, if we can easily send a 32-bit hash of the 128-bit UUID, > that'll make it easier to check for the popularity of non-standard services, > which would be better. How do we do that with the histogram macros? They seem to > always want a maximum+1 that fits in an int, and they allocate 'maximum' ints, > which is a lot of memory. The UMA macro for a sparse histogram is UMA_HISTOGRAM_SPARSE_SLOWLY() and doesn't require a max value param. It uses a map behind the scenes - unlike the other macros that pre-allocate the bucket array. The macro supports an int32 range - although it's a bit nicer to use 1 bit less to ensure the values are positive (make histograms.xml a bit more pleasant). Here's a few example hash functions people use: https://code.google.com/p/chromium/codesearch#chromium/src/chrome_elf/dll_has... https://code.google.com/p/chromium/codesearch#chromium/src/components/proximi... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo... (Makes me think that we should maybe make a version of that macro that takes a string param and does the hashing itself, rather than everyone re-inventing their own hash.)
Patchset #5 (id:180001) has been deleted
@asvitkine: Ready for review! https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:38: const uint16_t kMaxStandardServiceUUID = 0x22; On 2015/08/10 at 22:18:56, Jeffrey Yasskin wrote: > Let's not force ourselves to update this every time they come up with a new service UUID. Just pass the full 2 hexdigits? > > However, a constant for the 0x101 would be useful. Changing to hash function instead. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:73: if (!service.canonical_value().compare(0, 6, On 2015/08/10 at 22:18:56, Jeffrey Yasskin wrote: > Please use ==0 for .compare(). It's a tri-valued function, not a boolean, and ! tends to be confusing. Sorry. Should have looked at the documentation closer. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: 8, 28, kBaseStandardServiceUUID)) { // -0000-1000-8000-00805f9b34fb On 2015/08/10 at 22:18:56, Jeffrey Yasskin wrote: > This comparison isn't quite right. kBaseStandardServiceUUID will be compared from the beginning, but you want to compare from kBaseStandardServiceUUID + 8. Can we have a unittest for this? Changing to hash based histogram. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:82: service_enum <<= 4; On 2015/08/11 at 15:15:08, Alexei Svitkine wrote: > On 2015/08/10 22:18:56, Jeffrey Yasskin wrote: > > On 2015/08/10 22:03:35, Alexei Svitkine wrote: > > > Instead of doing this complicated logic, have you considered just logging the > > > hash of service.canonical_value() in an enumerated histogram> > > > > We didn't consider that, but 1) what's the maximum value for that enumeration? > > 2) The BT standards body gives us a contiguous range of values; it seems odd to > > spread them out and risk collisions by hashing. > > > > However, we don't need a loop here. > > > > const std::string& uuid = service.canonical_value(); > > uint16_t service_enum = base::HexDigitToInt(uuid[6]) * 0x10 + > > base::HexDigitToInt(uuid[7]); > > > > should be enough. We could CHECK_EQ(36, uuid.size()) if that makes anyone more > > comfortable. > > > > On the other hand, if we can easily send a 32-bit hash of the 128-bit UUID, > > that'll make it easier to check for the popularity of non-standard services, > > which would be better. How do we do that with the histogram macros? They seem to > > always want a maximum+1 that fits in an int, and they allocate 'maximum' ints, > > which is a lot of memory. > > The UMA macro for a sparse histogram is UMA_HISTOGRAM_SPARSE_SLOWLY() and doesn't require a max value param. It uses a map behind the scenes - unlike the other macros that pre-allocate the bucket array. > > The macro supports an int32 range - although it's a bit nicer to use 1 bit less to ensure the values are positive (make histograms.xml a bit more pleasant). > > Here's a few example hash functions people use: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome_elf/dll_has... > https://code.google.com/p/chromium/codesearch#chromium/src/components/proximi... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo... > > (Makes me think that we should maybe make a version of that macro that takes a string param and does the hashing itself, rather than everyone re-inventing their own hash.) Done. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:83: service_enum += base::HexDigitToInt(service.canonical_value()[i + 6]); On 2015/08/10 at 22:18:56, Jeffrey Yasskin wrote: > On 2015/08/10 22:03:36, Alexei Svitkine wrote: > > What guarantees that i+6 is in range? > > https://code.google.com/p/chromium/codesearch/#chromium/src/device/bluetooth/..., but it'd make sense to CHECK. No longer being used. https://codereview.chromium.org/1261593004/diff/160001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:89: return ++service_enum; On 2015/08/10 at 22:03:35, Alexei Svitkine wrote: > This should be service_enum + 1, since incrementing the local doesn't do anything here. Removed. https://codereview.chromium.org/1261593004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1261593004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2722: -<histogram name="Bluetooth.RequestDevice.Outcome" On 2015/08/10 at 22:03:36, Alexei Svitkine wrote: > It's better to mark the existing histogram <obsolete> so that old data can still be seen on the dashboard. Unless no data has been collected for this yet. Done.
https://codereview.chromium.org/1261593004/diff/200001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/200001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:95: UMA_HISTOGRAM_ENUMERATION( Use UMA_HISTOGRAM_SPARSE_SLOWLY here and on line 84.
https://codereview.chromium.org/1261593004/diff/200001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/200001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:95: UMA_HISTOGRAM_ENUMERATION( On 2015/08/11 at 20:32:36, Alexei Svitkine wrote: > Use UMA_HISTOGRAM_SPARSE_SLOWLY here and on line 84. Done. Realized as soon as I submitted. Sorry!
lgtm https://codereview.chromium.org/1261593004/diff/220001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/220001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:14: #include "base/metrics/histogram.h" Nit: Change this to histogram_macros.h
@asvitkine: Thanks! @jyasskin: PTAL https://codereview.chromium.org/1261593004/diff/220001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/220001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:14: #include "base/metrics/histogram.h" On 2015/08/11 at 20:40:23, Alexei Svitkine wrote: > Nit: Change this to histogram_macros.h Done.
lgtm with the changes below. https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:79: UMA_HISTOGRAM_COUNTS("Bluetooth.Web.RequestDevice.Filters.Count", Should these be UMA_HISTOGRAM_COUNTS_100? We're not expecting 100 filters or 100 services, let alone the 1000000 that UMA_HISTOGRAM_COUNTS() allows. https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:105: std::set<BluetoothUUID> union_of_services(optional_services.begin(), I'm a little worried about the duplicate expense of computing this union, which we also compute most of in ComputeScanFilter(), but we can probably wait until it shows up on a profile before really trying to optimize it. https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2741: enum="BluetoothRequestDeviceOutcome"> Would it make sense to have this use the WebBluetoothRequestDeviceOutcome enum instead of having 2 enums? They have the same values right now, right? https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2781: + Records what GATT Services are used when scanning. This will help us know if Mention that these are the 31-bit hashes of the service UUID strings. https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2816: + Records how many optional services are used when scanning. This results will These too.
https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:79: UMA_HISTOGRAM_COUNTS("Bluetooth.Web.RequestDevice.Filters.Count", On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: > Should these be UMA_HISTOGRAM_COUNTS_100? We're not expecting 100 filters or 100 services, let alone the 1000000 that UMA_HISTOGRAM_COUNTS() allows. Changed all instances of COUNTS to COUNTS_100 https://codereview.chromium.org/1261593004/diff/240001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:105: std::set<BluetoothUUID> union_of_services(optional_services.begin(), On 2015/08/11 at 21:10:16, Jeffrey Yasskin wrote: > I'm a little worried about the duplicate expense of computing this union, which we also compute most of in ComputeScanFilter(), but we can probably wait until it shows up on a profile before really trying to optimize it. Ack. https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2741: enum="BluetoothRequestDeviceOutcome"> On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: > Would it make sense to have this use the WebBluetoothRequestDeviceOutcome enum instead of having 2 enums? They have the same values right now, right? That's what I had at the beginning but saw a couple of histograms that deprecated both the enum and histogram in favor or a similar enum and histogram so I went with that[1]. If you have a strong preference I can change it back. [1] https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2781: + Records what GATT Services are used when scanning. This will help us know if On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: > Mention that these are the 31-bit hashes of the service UUID strings. Done. https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2816: + Records how many optional services are used when scanning. This results will On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: > These too. Done.
lgtm except for the misplaced hash comments. https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2741: enum="BluetoothRequestDeviceOutcome"> On 2015/08/11 21:53:27, ortuno wrote: > On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: > > Would it make sense to have this use the WebBluetoothRequestDeviceOutcome enum > instead of having 2 enums? They have the same values right now, right? > > That's what I had at the beginning but saw a couple of histograms that > deprecated both the enum and histogram in favor or a similar enum and histogram > so I went with that[1]. If you have a strong preference I can change it back. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... I do not have a strong preference. https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2793: + is a 31-bit hash of the Service UUID. This results will help us better This one isn't a hash. ;) https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2817: + Records how many optional services are used when scanning. This results will I think you missed mentioning the hash here.
If the two enums are identical, I thinking its better to not duplicate the definition. On 11 Aug 2015 5:57 pm, <jyasskin@chromium.org> wrote: > lgtm except for the misplaced hash comments. > > > > https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (left): > > > https://codereview.chromium.org/1261593004/diff/240001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:2741: > enum="BluetoothRequestDeviceOutcome"> > On 2015/08/11 21:53:27, ortuno wrote: > >> On 2015/08/11 at 21:10:17, Jeffrey Yasskin wrote: >> > Would it make sense to have this use the >> > WebBluetoothRequestDeviceOutcome enum > >> instead of having 2 enums? They have the same values right now, right? >> > > That's what I had at the beginning but saw a couple of histograms that >> deprecated both the enum and histogram in favor or a similar enum and >> > histogram > >> so I went with that[1]. If you have a strong preference I can change >> > it back. > > [1] >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > > I do not have a strong preference. > > > https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > > https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:2793: + is a 31-bit hash of > the Service UUID. This results will help us better > This one isn't a hash. ;) > > > https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:2817: + Records how many > optional services are used when scanning. This results will > I think you missed mentioning the hash here. > > https://codereview.chromium.org/1261593004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2793: + is a 31-bit hash of the Service UUID. This results will help us better On 2015/08/11 at 21:57:50, Jeffrey Yasskin wrote: > This one isn't a hash. ;) (^_^;) https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2817: + Records how many optional services are used when scanning. This results will On 2015/08/11 at 21:57:50, Jeffrey Yasskin wrote: > I think you missed mentioning the hash here. Done. https://codereview.chromium.org/1261593004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:52144: <enum name="BluetoothRequestDeviceOutcome" type="int"> Removed.
@scheib: PTAL
LGTM https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:69: uint32 data = base::Hash(service.canonical_value()); base::Hash uses SuperFastHash, which has a lot of collisions for shorter strings. We avoided using it in GetPeripheralHashAddress https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... http://programmers.stackexchange.com/questions/49550/which-hashing-algorithm-... discusses this some. OK, all that said I think base::Hash is still probably what we want to use here. https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:328: VLOG(1) << "["; Maybe "\t[" and "\t\t" << service.value()
https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:69: uint32 data = base::Hash(service.canonical_value()); On 2015/08/12 at 21:00:36, scheib wrote: > base::Hash uses SuperFastHash, which has a lot of collisions for shorter strings. We avoided using it in GetPeripheralHashAddress https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > http://programmers.stackexchange.com/questions/49550/which-hashing-algorithm-... discusses this some. OK, all that said I think base::Hash is still probably what we want to use here. SuperFastHash seems to be OK for UUIDs i.e. 4 collisions in 216,553 UUIDs https://codereview.chromium.org/1261593004/diff/300001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:328: VLOG(1) << "["; On 2015/08/12 at 21:00:35, scheib wrote: > Maybe > "\t[" and > "\t\t" << service.value() Done.
Replace base::Hash with base::SuperFastHash. Also added comments to use the new histogram macro for fingerprints.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261593004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261593004/340001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1261593004/#ps340001 (title: "Use SuperFastHash directly.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261593004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261593004/340001
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f61dcf839472db5b0a8a3dc0f241929f17ddb8ff Cr-Commit-Position: refs/heads/master@{#343139} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
