Closed Bug 1128472 Opened 5 years ago Closed 4 years ago

Telemetry Environment: add CPU Vendor, Family and Stepping per environment spec

Categories

(Toolkit :: Telemetry, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox43 --- verified

People

(Reporter: Dexter, Assigned: milan)

References

Details

(Whiteboard: [unifiedTelemetry])

Attachments

(3 files, 15 obsolete files)

15.25 KB, patch
milan
: review+
Details | Diff | Splinter Review
4.01 KB, patch
milan
: review+
Details | Diff | Splinter Review
11.69 KB, patch
milan
: review+
Details | Diff | Splinter Review
We should add CPU Vendor, Family, Model and Stepping to the Telemetry Environment as per the Environment Spec (bug 1069880).

I can think of two approaches:

1) Using ctypes in TelemetryEnvironment.jsm, defining a function for each platform.

2) Exposing the data through |Services.sysinfo|, probably by modifying [2].

[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js#272
[2] - https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp
Blocks: 1122052
Blocks: 1121568
No longer blocks: 1122052
Depends on: 1122052
Blocks: 1120356
No longer blocks: 1121568
(In reply to Alessio Placitelli [:Dexter] from comment #0)
> We should add CPU Vendor, Family, Model and Stepping to the Telemetry
> Environment as per the Environment Spec (bug 1069880).
> 
> I can think of two approaches:
> 
> 1) Using ctypes in TelemetryEnvironment.jsm, defining a function for each
> platform.
> 
> 2) Exposing the data through |Services.sysinfo|, probably by modifying [2].

Nathan, does 2) sound reasonable for you?
Any specific things to watch out for with this?
Flags: needinfo?(nfroyd)
Exposing the data through Services.sysinfo sounds much more reasonable than trying to do it via ctypes; you'd have to write C++ code for ctypes anyway, I think, so you might as well do it in nsSystemInfo instead.

All that information is really detailed, though...what is it buying you that the current acquisition of SSE/AVX/etc. capabilities isn't providing?  I think it makes sense for crashes (some crashes only occur on particular manufacturers's CPUs), but how would it be useful for general-purpose telemetry?
Flags: needinfo?(nfroyd) → needinfo?(gfritzsche)
This was requested by Benjamin, moving the ni?.
Flags: needinfo?(gfritzsche) → needinfo?(benjamin)
We're unifying a lot of the analysis infrastructure, and measuring crash counts in telemetry, so having the same aggregating data we're using in crash-stats is important. This may also drive self-support tips about the broken AMD CPUs.
Flags: needinfo?(benjamin)
Whiteboard: [help]
Whiteboard: [help] → [help] [not a blocker]
Duplicate of this bug: 1170383
Blocks: 1122482
No longer blocks: 1120356
Is it enough to just have the whole "info" string like crash report, e.g., "
GenuineIntel family 6 model 15 stepping 13", or do we want this parsed out into separate fields as in the placeholder telemetry code?
Comment on attachment 8645172 [details] [diff] [review]
(WIP) Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.

Review of attachment 8645172 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

The test coverage for this to update is here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#365

... and the documentation updates would go in here (note ones platform availability and other potential caveats would be great):
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/environment.rst

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1063,5 @@
> +      vendor: getSysinfoProperty("cpuvendor", null),
> +      family: getSysinfoProperty("cpufamily", null),
> +      model: getSysinfoProperty("cpumodel", null),
> +      stepping: getSysinfoProperty("cpustepping", null),
> +      cacheMB: getSysinfoProperty("cpucache", null),

I'm not sure what the cache sizes in machines currently used in the wild are.
Is MB precision sufficient for our needs?
Attachment #8645172 - Flags: feedback+
Component: Client: Desktop → Telemetry
Product: Firefox Health Report → Toolkit
Whiteboard: [help] [not a blocker] → [unifiedTelemetry]
KB for cache, otherwise the same patch.  Part 1, though I'll probably land this while the rest of the parameters and OSs are getting worked on.
Attachment #8645172 - Attachment is obsolete: true
Attachment #8650602 - Flags: review?(gfritzsche)
Assignee: nobody → milan
Status: NEW → ASSIGNED
Comment on attachment 8650602 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8650602 [details] [diff] [review]:
-----------------------------------------------------------------

Are the documentation & test updates coming up in a separate patch?
We should not land this without these.

Aaron, can you take a quick look over the Windows nsSystemInfo.cpp changes?
It's been a while since i touched those APIs.
Attachment #8650602 - Flags: feedback?(aklotz)
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8650602 [details] [diff] [review]
> Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win,
> virtual memory max on Win, vendor on Mac.   r=gfritzsche
> 
> Review of attachment 8650602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are the documentation & test updates coming up in a separate patch?
> We should not land this without these.

Right, sorry, I'll get the documentation and test updates in a separate patch and not land anything without them.
Testing this on my MBP i get:
  cpu.count 	8
  cpu.cores 	4
  cpu.vendor 	GenuineIntel
  cpu.family 	6
  cpu.model 	70
  cpu.stepping 	1
  cpu.cacheKB 	8192
  cpu.speedMHz 	2600

... vs. this info from the "system information" tool:
  Model Name:	MacBook Pro
  Model Identifier:	MacBookPro11,3
  Processor Name:	Intel Core i7
  Processor Speed:	2,6 GHz
  Number of Processors:	1
  Total Number of Cores:	4
  L2 Cache (per Core):	256 KB
  L3 Cache:	6 MB
  Memory:	16 GB

So, apparently we fail to retrieve the L3 cache and report the L2 cache instead?
Should we maybe just report both L3 & L2 cache where available?
(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> ...
> 
> So, apparently we fail to retrieve the L3 cache and report the L2 cache
> instead?
> Should we maybe just report both L3 & L2 cache where available?

That's a good point.  Yes, we try L3, if we don't find the information, we take L2.  I don't have a strong opinion on which number to report here (L2 only, L3 ony, sum of the two, L2 and L3 separately.)  I'm also not sure we're going to make serious decisions based on that piece of telemetry information anyway :)

Any strong feelings one way or the other?  There doesn't seem to be information at that level of detail in this or bug 1069880, or any of the documents attached to it.
For example, on Windows we just report the largest cache.
Comment on attachment 8653558 [details] [diff] [review]
Part 1B. Tests and documentation for Part 1.

Review of attachment 8653558 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: toolkit/components/telemetry/docs/environment.rst
@@ +79,5 @@
>          cpu: {
> +            count: <number>,  // e.g. 8, or null on failure - logical cpus
> +            cores: <number>, // e.g., 4, or null on failure - physical cores, windows, mac only
> +            vendor: <string>, // e.g. "GenuineIntel", or null on failure, mac only
> +            family: <string>, // null on failure, windows, mac only

Nit: "windows & mac only"? I read it as "mac only" on first look.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +369,5 @@
> +  if (gIsWindows || gIsMac) {
> +    if (gIsWindows) {
> +      EXTRA_CPU_FIELDS = ["cores", "model", "family", "stepping", "cacheKB", "speedMHz"];
> +    } else {
> +      // This is gIsMac:

Nit: If we write it here, might as well explicitly use |else if (gIsMac)|?
Attachment #8653558 - Flags: review?(gfritzsche) → review+
I don't have a strong opinion on how we report caches, as long as what we report is consistent. Always L2, or always L3, or always L2+L3 where some of these can be null.

Note that if we have the cpumodel, we can obtain the full cache info from Intel's and AMD's sites and use these in analyses.
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #18)
> I don't have a strong opinion on how we report caches, as long as what we
> report is consistent. Always L2, or always L3, or always L2+L3 where some of
> these can be null.

Ok, i'd say lets go for the last option, then we are covered either way.
Just reporting the sum gets us into interesting situations; for example about:telemetry would show 9472KB for the system I have with Windows (8 * 32KB + 4 * 256KB + 8MB), on a Xeon E3-1270 v2 that shows "Intel Smart Cache 8 MB".  L1 would show up per logical core, L2 per physical core, L3 once.  What would be the right value to show there?
Sorry, that means both separately, with a value of null where they are not present.
Attachment #8650602 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> Testing this on my MBP i get:
> ...
> So, apparently we fail to retrieve the L3 cache...

That's a bug in the patch above.
Compared to the previous version of the patch:

The test is the same as the r+'d version, with nits addressed and accounting for L2 + L3 caches.
Added L2 and L3 cache separately.
Fixed some 32-bit/64-bit mixups on the Mac side, which is why L3 cache didn't work.  Put in a bunch of asserts to catch similar problems.
Attachment #8650602 - Attachment is obsolete: true
Attachment #8653558 - Attachment is obsolete: true
Attachment #8650602 - Flags: feedback?(aklotz)
Attachment #8653664 - Flags: review?(gfritzsche)
Comment on attachment 8653664 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8653664 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good to me.
I'll defer the Windows-specific nsSystemInfo.cpp changes to aklotz.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1089,5 @@
> +      family: getSysinfoProperty("cpufamily", null),
> +      model: getSysinfoProperty("cpumodel", null),
> +      stepping: getSysinfoProperty("cpustepping", null),
> +      L2cacheKB: getSysinfoProperty("cpucachel2", null),
> +      L3cacheKB: getSysinfoProperty("cpucachel3", null),

Lets use consistent camel-case, i.e. l2CacheKB, l3CacheKB?

::: xpcom/base/nsSystemInfo.cpp
@@ +246,5 @@
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> +  *physical_cpus = 0;
> +  *cache_size_L2 = 0; // This will be in kbytes
> +  *cache_size_L3 = 0; // This will be in kbytes

Style nit: The surrounding code uses camel-case.

@@ +259,5 @@
> +  // Determine buffer size, allocate and get processor information.
> +  // Size can change between calls (unlikely), so a loop is done.
> +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION infoBuffer[32];
> +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION* infos = &infoBuffer[0];
> +  DWORD return_length = sizeof(infoBuffer);

Style nit: The surrounding code uses camel-case.
Attachment #8653664 - Flags: review?(gfritzsche)
Attachment #8653664 - Flags: review?(aklotz)
Attachment #8653664 - Flags: review+
Comment on attachment 8653664 [details] [diff] [review]
Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win, virtual memory max on Win, vendor on Mac.   r=gfritzsche

Review of attachment 8653664 [details] [diff] [review]:
-----------------------------------------------------------------

OK with our nits addressed.

::: xpcom/base/nsSystemInfo.cpp
@@ +243,5 @@
> +// Based on media/webrtc/trunk/webrtc/base/systeminfo.cc
> +typedef BOOL (WINAPI *LPFN_GLPI)(
> +    PSYSTEM_LOGICAL_PROCESSOR_INFORMATION,
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {

style nit: static void on separate line from the rest of the signature. Open curly brace on its own line.

@@ +244,5 @@
> +typedef BOOL (WINAPI *LPFN_GLPI)(
> +    PSYSTEM_LOGICAL_PROCESSOR_INFORMATION,
> +    PDWORD);
> +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> +  *physical_cpus = 0;

Let's add some assertions to the top of this function to verify that a caller hasn't passed nullptr to any of these paramaters.

@@ +252,5 @@
> +  // GetLogicalProcessorInformation() is available on Windows XP SP3 and beyond.
> +  LPFN_GLPI glpi = reinterpret_cast<LPFN_GLPI>(GetProcAddress(
> +      GetModuleHandle(L"kernel32"),
> +      "GetLogicalProcessorInformation"));
> +  if (NULL == glpi) {

s/NULL/nullptr/
Attachment #8653664 - Flags: review?(aklotz) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Comment on attachment 8653664 [details] [diff] [review]
> Part 1. Family, model, stepping, cores, cache, cpu speed on Mac on Win,
> virtual memory max on Win, vendor on Mac.   r=gfritzsche
> 
> Review of attachment 8653664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks good to me.
> I'll defer the Windows-specific nsSystemInfo.cpp changes to aklotz.
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +1089,5 @@
> > +      family: getSysinfoProperty("cpufamily", null),
> > +      model: getSysinfoProperty("cpumodel", null),
> > +      stepping: getSysinfoProperty("cpustepping", null),
> > +      L2cacheKB: getSysinfoProperty("cpucachel2", null),
> > +      L3cacheKB: getSysinfoProperty("cpucachel3", null),
> 
> Lets use consistent camel-case, i.e. l2CacheKB, l3CacheKB?

I'll make that change.

> 
> ::: xpcom/base/nsSystemInfo.cpp
> @@ +246,5 @@
> > +    PDWORD);
> > +static void GetProcessorInformation(int* physical_cpus, int* cache_size_L2, int* cache_size_L3) {
> > +  *physical_cpus = 0;
> > +  *cache_size_L2 = 0; // This will be in kbytes
> > +  *cache_size_L3 = 0; // This will be in kbytes
> 
> Style nit: The surrounding code uses camel-case.
> 
> @@ +259,5 @@
> > +  // Determine buffer size, allocate and get processor information.
> > +  // Size can change between calls (unlikely), so a loop is done.
> > +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION infoBuffer[32];
> > +  SYSTEM_LOGICAL_PROCESSOR_INFORMATION* infos = &infoBuffer[0];
> > +  DWORD return_length = sizeof(infoBuffer);
> 
> Style nit: The surrounding code uses camel-case.

I'll fix the infoBuffer to be info_buffer, but I'll leave the other things with underscores - since this function pretty much unchanged came from webrtc code, I wanted to leave the code looking like the original just to make it easier to notice if anything changes.  Not a very solid argument though.
With (most, except the ones explained away in above comment) nits addressed.
Attachment #8653664 - Attachment is obsolete: true
Attachment #8654380 - Flags: review+
Tweak the test a bit to cover scenarios where we don't have L2 and L3 cache.
Attachment #8654380 - Attachment is obsolete: true
Attachment #8654950 - Flags: review+
Windows wants explicit <string> include.
Attachment #8654384 - Attachment is obsolete: true
Attachment #8655071 - Attachment is obsolete: true
Make it linux GTK specific, that was the intent.
Attachment #8655663 - Attachment is obsolete: true
Priority: -- → P2
Don't know where to find cpu frequency on all Linux systems, so leave it optional when unit testing.
Attachment #8655679 - Attachment is obsolete: true
Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Lee, is this "linux-e" enough?
Attachment #8656183 - Flags: feedback?(lsalzman)
Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Review of attachment 8656183 [details] [diff] [review]:
-----------------------------------------------------------------

Looks tolerabl-e linux-e.
Attachment #8656183 - Flags: feedback?(lsalzman) → feedback+
Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Same as the first patch was doing for windows + mac.
Attachment #8656183 - Flags: review?(gfritzsche)
Comment on attachment 8656183 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.

Review of attachment 8656183 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the Telemetry changes with the comments addressed.
I'm not sure if we should use that - apparently abandoned - Tokenizer given the other implementation that is actually used.

::: toolkit/components/telemetry/docs/environment.rst
@@ +78,5 @@
>          virtualMaxMB: <number>, // windows-only
>          isWow64: <bool>, // windows-only
>          cpu: {
>              count: <number>,  // e.g. 8, or null on failure - logical cpus
> +            cores: <number>, // e.g., 4, or null on failure - physical cores

"desktop-only"
Ditto for the others below.

::: toolkit/components/telemetry/tests/unit/head.js
@@ +13,5 @@
>  const gIsMac = ("@mozilla.org/xpcom/mac-utils;1" in Cc);
>  const gIsAndroid =  ("@mozilla.org/android/bridge;1" in Cc);
>  const gIsGonk = ("@mozilla.org/cellbroadcast/gonkservice;1" in Cc);
> +const gIsLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc) ||
> +                 ("@mozilla.org/gio-service;1" in Cc);

The changes here don't seem Gnome specific - i think we should instead use the newish AppConstants.jsm and check AppConstants.platform=="linux".

::: xpcom/ds/Tokenizer.h
@@ +323,5 @@
> +  // and value is separated with a colon (by default.)  The separator should
> +  // not be used elsewhere in the key or value entry.
> +  // Same functionality and limitations as the Tokenizer it uses.
> +  //
> +  static bool ParseConfigFile(const std::string& aFilename,

* (ConfigFile seems a bit generic, ParseKeyValueFile?)
* Tokenizer.h isn't included anywhere AFAICT, might be unused?
* we have nsCharSeparatedTokenizer, which is actually used across the tree
* maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a helper function in nsSystemInfo.cpp?
Attachment #8656183 - Flags: review?(gfritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #42)
> ...
> * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> * we have nsCharSeparatedTokenizer, which is actually used across the tree
> * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> helper function in nsSystemInfo.cpp?

I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.
(In reply to Milan Sreckovic [:milan] from comment #43)
> (In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment
> #42)
> > ...
> > * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> > * we have nsCharSeparatedTokenizer, which is actually used across the tree
> > * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> > helper function in nsSystemInfo.cpp?
> 
> I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.

By the way, the "unused" part of the Tokenizer - it only showed up about 6 weeks ago, so it isn't surprising.  This blogpost describes it: http://www.janbambas.cz/string-parsing-made-simple-with-mozillatokenizer/
(In reply to Milan Sreckovic [:milan] from comment #44)
> (In reply to Milan Sreckovic [:milan] from comment #43)
> > (In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment
> > #42)
> > > ...
> > > * Tokenizer.h isn't included anywhere AFAICT, might be unused?
> > > * we have nsCharSeparatedTokenizer, which is actually used across the tree
> > > * maybe we can just use nsCharSeparatedTokenizer and wrap its usage in a
> > > helper function in nsSystemInfo.cpp?
> > 
> > I'll take a look at nsCharSeparatedTokenizer, thanks for the pointer.
> 
> By the way, the "unused" part of the Tokenizer - it only showed up about 6
> weeks ago, so it isn't surprising.  This blogpost describes it:
> http://www.janbambas.cz/string-parsing-made-simple-with-mozillatokenizer/

Ah, i see - maybe we should use the cleaner of the two then.

Note that i'll be out from tomorrow afternoon on - :vladan or :dexter should be good for Telemetry related questions/flags.
Comment on attachment 8655664 [details] [diff] [review]
Part 3. Vendor string on windows.

Review of attachment 8655664 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fix applied.

::: xpcom/base/nsSystemInfo.cpp
@@ +389,5 @@
> +                         reinterpret_cast<LPBYTE>(cpuVendorStr),
> +                         &len) == ERROR_SUCCESS &&
> +        vtype == REG_SZ && len % 2 == 0 && len > 1) {
> +      cpuVendorStr[len/2] = 0; // In case it isn't null terminated
> +      CopyUTF16toUTF8(nsAutoString(cpuVendorStr), cpuVendor);

s/nsAutoString/nsDependentString/
Attachment #8655664 - Flags: review?(aklotz) → review+
Nits, plus, used the nsCharSeparatedTokenizer for the overall parsing, made the code simpler, and no changes to the (new and unrelated) Tokenizer class, though the later came in handy for some simple parsing within the values.
Attachment #8656183 - Attachment is obsolete: true
Attachment #8656770 - Flags: review+
Use the right string type.
Attachment #8655664 - Attachment is obsolete: true
Attachment #8656771 - Flags: review+
Comment on attachment 8656770 [details] [diff] [review]
Part 2. Linux support for the items from Part 1.  Carry r=gfritzsche

Review of attachment 8656770 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsSystemInfo.cpp
@@ +85,5 @@
> +      key = tokens.nextToken();
> +      if (tokens.hasMoreTokens()) {
> +        value = tokens.nextToken();
> +      }
> +      aKeyValuePairs[key] = value;

Isn't it enough to only insert if we found two tokens?

@@ +485,5 @@
> +    // cpuVendor from "vendor_id"
> +    cpuVendor.Assign(keyValuePairs[NS_LITERAL_CSTRING("vendor_id")]);
> +
> +    // cpuFamily from "cpu family"
> +    cpuFamily = atoi(keyValuePairs[NS_LITERAL_CSTRING("cpu family")].get());

Theoretical failure scenario because of missing conversion failure checks:
If the converted value for atoi is outside the target types range, the return value is undefined.

Ditto the others below.

@@ +501,5 @@
> +      // cacheSizeL3 from "cache size"
> +      Tokenizer::Token t;
> +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());

No range checks?

@@ +503,5 @@
> +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> +            t.AsString() != NS_LITERAL_CSTRING("KB")) {

Can you add a comment on what this does?

@@ +504,5 @@
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> +          cacheSizeL3 = 13;

Whats up with the specific value 13 here?

@@ +518,5 @@
> +    if (getline(input, line)) {
> +      Tokenizer::Token t;
> +      Tokenizer p(line.c_str());
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cpuSpeed = static_cast<int>(t.AsInteger()/1000);

No range check?

@@ +531,5 @@
> +    if (getline(input, line)) {
> +      Tokenizer::Token t;
> +      Tokenizer p(line.c_str(), nullptr, "K");
> +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> +        cacheSizeL2 = static_cast<int>(t.AsInteger());

No range check?
(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #49)
> Comment on attachment 8656770 [details] [diff] [review]
> Part 2. Linux support for the items from Part 1.  Carry r=gfritzsche
> 
> Review of attachment 8656770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsSystemInfo.cpp
> @@ +85,5 @@
> > +      key = tokens.nextToken();
> > +      if (tokens.hasMoreTokens()) {
> > +        value = tokens.nextToken();
> > +      }
> > +      aKeyValuePairs[key] = value;
> 
> Isn't it enough to only insert if we found two tokens?

No, there are cases where we have the key, and the value is blank, and we want to record the blank.

> 
> @@ +485,5 @@
> > +    // cpuVendor from "vendor_id"
> > +    cpuVendor.Assign(keyValuePairs[NS_LITERAL_CSTRING("vendor_id")]);
> > +
> > +    // cpuFamily from "cpu family"
> > +    cpuFamily = atoi(keyValuePairs[NS_LITERAL_CSTRING("cpu family")].get());
> 
> Theoretical failure scenario because of missing conversion failure checks:
> If the converted value for atoi is outside the target types range, the
> return value is undefined.
> 
> Ditto the others below.

Right, those would just get returned as 0.  We can see if the telemetry than has any of those values, and worry about it at that point?

> 
> @@ +501,5 @@
> > +      // cacheSizeL3 from "cache size"
> > +      Tokenizer::Token t;
> > +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> 
> No range checks?

As in, if the cache returned is outside of the range of 32-bit integer?  Given that we're just using this value to display the cache, I'm not horribly worried about it.


> 
> @@ +503,5 @@
> > +      Tokenizer p(keyValuePairs[NS_LITERAL_CSTRING("cache size")]);
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> > +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> > +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> 
> Can you add a comment on what this does?

Will do.

> 
> @@ +504,5 @@
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL3 = static_cast<int>(t.AsInteger());
> > +        if (p.Next(t) && t.Type() == Tokenizer::TOKEN_WORD &&
> > +            t.AsString() != NS_LITERAL_CSTRING("KB")) {
> > +          cacheSizeL3 = 13;
> 
> Whats up with the specific value 13 here?

Nothing that makes any sense, just a leftover from debugging.  I'll clean it up.

> 
> @@ +518,5 @@
> > +    if (getline(input, line)) {
> > +      Tokenizer::Token t;
> > +      Tokenizer p(line.c_str());
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cpuSpeed = static_cast<int>(t.AsInteger()/1000);
> 
> No range check?
> 
> @@ +531,5 @@
> > +    if (getline(input, line)) {
> > +      Tokenizer::Token t;
> > +      Tokenizer p(line.c_str(), nullptr, "K");
> > +      if (p.Next(t) && t.Type() == Tokenizer::TOKEN_INTEGER) {
> > +        cacheSizeL2 = static_cast<int>(t.AsInteger());
> 
> No range check?


How strongly do you feel about the int64->int32 range checks for this code?  I can comment that we don't care, or I can put the range check in.  I'm leaning towards the former, but it's your r+ on it, so I'm happy to add the range check if you think we need it.
Address the review comments - now with range checks, tokenizer to avoid atoi direct calls, comments about some of the potentially confusing code.
Attachment #8656770 - Attachment is obsolete: true
Attachment #8658382 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbc063e8edf

(In reply to Milan Sreckovic [:milan] from comment #50)
> ...
> > No range check?
> 
> How strongly do you feel about the int64->int32 range checks for this code? 

Never mind the question, added the range checks.
Keywords: checkin-needed
Attachment #8656771 - Attachment is patch: true
I have reproduced this bug on Nightly 38.0a1 (2015-02-02) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 43.0b3!

Build ID: 20151112144305
User Agent: Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0

[testday-20151113]
I have reproduced this on Firefox nightly 38.0a1 according to (2015-2-2)

It's verified on Latest Firefox Beta
Build ID 	20151123113812
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS- 32bit windows8.1
QA Whiteboard: [testday-20151127]
Status: RESOLVED → VERIFIED
Depends on: 1284167
You need to log in before you can comment on or make changes to this bug.