Last Comment Bug 657297 - Telemetry probe for preloading
: Telemetry probe for preloading
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All Windows 7
-- normal (vote)
: mozilla7
Assigned To: Nobody; OK to take it and work on it
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on: 632404 657709 658854 661574
Blocks: 659028 659396
  Show dependency treegraph
 
Reported: 2011-05-16 01:21 PDT by Mike Hommey [:glandium]
Modified: 2011-06-22 10:18 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Expose a function to add telemetry samples in XRE (2.00 KB, patch)
2011-05-17 08:57 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch (2.05 KB, patch)
2011-05-17 09:01 PDT, Mike Hommey [:glandium]
taras.mozilla: review-
Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE (3.91 KB, patch)
2011-05-18 02:37 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch (5.27 KB, patch)
2011-05-18 06:11 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.07 KB, patch)
2011-05-19 04:24 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.95 KB, patch)
2011-05-19 04:30 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.95 KB, patch)
2011-05-19 07:46 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.96 KB, patch)
2011-05-19 08:56 PDT, Mike Hommey [:glandium]
benjamin: superreview+
Details | Diff | Splinter Review
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch (4.64 KB, patch)
2011-05-20 04:52 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.96 KB, patch)
2011-05-21 23:20 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.90 KB, patch)
2011-05-22 22:19 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 2 - Add telemetry samples for initial I/O counters. (5.15 KB, patch)
2011-05-22 22:20 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (4.90 KB, patch)
2011-05-23 08:08 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 1 - Expose a function to add telemetry samples in XRE. (1.77 KB, patch)
2011-06-16 22:52 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 2 - Add telemetry samples for initial I/O counters. (6.56 KB, patch)
2011-06-16 22:53 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review

Description User image Mike Hommey [:glandium] 2011-05-16 01:21:57 PDT
This is about
https://bug627591.bugzilla.mozilla.org/attachment.cgi?id=530687
for bug 632404.
Comment 1 User image Mike Hommey [:glandium] 2011-05-17 08:57:55 PDT
Created attachment 532980 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE
Comment 2 User image Mike Hommey [:glandium] 2011-05-17 09:01:10 PDT
Created attachment 532984 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch
Comment 3 User image (dormant account) 2011-05-17 09:19:04 PDT
Comment on attachment 532984 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch

> #if defined(_MSC_VER) && defined(_M_IX86)
>   XRE_SetupDllBlocklist();
>+  XRE_TelemetryAdd("Startup::ProcessIoCounters.ReadOperationCount (0 == no Windows prefetch)",
>+                   (int) ioCounters.ReadOperationCount, 1, 100, 50);
>+  XRE_TelemetryAdd("Startup::ProcessIoCounters.ReadTransferCount (bytes)",
>+                   (int) ioCounters.ReadTransferCount, 1, 1000000, 50);
I know I set a bad example in my original patch for this.

10 buckets should be enough in both cases. Change the second number to KB.
Comment 4 User image Mike Hommey [:glandium] 2011-05-18 02:37:19 PDT
Created attachment 533223 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE

This exposes an API similar to that of bug 657709 (which it depends upon)
Comment 5 User image Mike Hommey [:glandium] 2011-05-18 06:11:21 PDT
Created attachment 533262 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch
Comment 6 User image (dormant account) 2011-05-18 13:32:46 PDT
Comment on attachment 533223 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE


>+  if (histogram_type == nsITelemetry::HISTOGRAM_EXPONENTIAL) {
>+    *aResult = Histogram::FactoryGet(name, min, max, bucket_count, Histogram::kNoFlags);
>+  } else {
>+    *aResult = LinearHistogram::FactoryGet(name, min, max, bucket_count, Histogram::kNoFlags);
>+  }
>+  return *aResult ? NS_OK : NS_ERROR_FAILURE;

This should be NS_OK. It's never null. 

What follows are optional nits:

>+}
>+
>+NS_IMETHODIMP
>+Telemetry::NewHistogram(const nsACString &name, PRUint32 min, PRUint32 max, PRUint32 bucket_count, PRUint32 histogram_type, JSContext *cx, jsval *ret)
>+{

A comment might be nice here.

/**
* This is for code outside of xul that can't use histogram.h directly.
*/

>   Histogram *h;
>-  if (histogram_type == nsITelemetry::HISTOGRAM_EXPONENTIAL) {
>-    h = Histogram::FactoryGet(name.BeginReading(), min, max, bucket_count, Histogram::kNoFlags);
>-  } else {
>-    h = LinearHistogram::FactoryGet(name.BeginReading(), min, max, bucket_count, Histogram::kNoFlags);
>-  }
>+  nsresult rv = HistogramGet(name.BeginReading(), min, max, bucket_count, histogram_type, &h);
>+  NS_ENSURE_SUCCESS(rv, rv);

For instance and the rest, we prefer explicit "if (NS_FAILED(rv)) return rv;" in new code.

Same in the rest of the patch.
Comment 7 User image (dormant account) 2011-05-18 14:34:08 PDT
Comment on attachment 533262 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch

>+struct jsval; // for nsITelemetry.h
>+#include "nsITelemetry.h" // for nsITelemetry::HISTOGRAM_* consts

Yuck. Det up an enum in nsXULAppAPI.h  that is equivalent to values of nsITelemetry::HISTOGRAM_. Those constants aren't gonna change.
 
 
>+  if (gotCounters) {
>+#if defined(XP_WIN)
>+    XRE_TelemetryAdd("BeforeMain::ProcessIoCounters.ReadOperationCount",
>+                     int(ioCounters.ReadOperationCount), 1, 100, 10, nsITelemetry::HISTOGRAM_LINEAR);
>+    XRE_TelemetryAdd("BeforeMain::ProcessIoCounters.ReadTransferCount (KiB)",
>+                     int(ioCounters.ReadTransferCount / 1024), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);

You max needs to be /1024 there. 10 * 1024 seems more reasonable to me. Otherwise the distribution into buckets is going to be rather narrow.

>+    IO_COUNTERS newIoCounters;
>+    if (GetProcessIoCounters(GetCurrentProcess(), &newIoCounters)) {
>+      XRE_TelemetryAdd("GlueStartup::ProcessIoCounters.ReadOperationCount",
>+                       int(newIoCounters.ReadOperationCount - ioCounters.ReadOperationCount), 1, 100, 10, nsITelemetry::HISTOGRAM_LINEAR);
>+      XRE_TelemetryAdd("GlueStartup::ProcessIoCounters.ReadTransferCount (KiB)",
>+                       int((newIoCounters.ReadTransferCount - ioCounters.ReadTransferCount) / 1024), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);

Same here

>+    }
>+#elif defined(XP_UNIX)
>+    XRE_TelemetryAdd("BeforeMain::RUsage::HardPageFaults",
>+                     int(initialRUsage.ru_majflt), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);

This max is way too high too. Shouldn't be more than 300. Ie 30MB/128KB



>+    struct rusage newRUsage;
>+    if (!getrusage(RUSAGE_SELF, &newRUsage)) {
>+      XRE_TelemetryAdd("GlueStartup::RUsage::HardPageFaults",
>+                       int(newRUsage.ru_majflt - initialRUsage.ru_majflt), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);

And here.


r+ with above adjusted

 
>+    }
>+#endif

Optional:

I would use KB to be consistent with existing telemetry probes and the rest of Firefox. We don't use KiB elsewhere.
I would also s/BeforeMain/Early.GlueStartup/ to be more specific(ie this isn't static-initializer-early). To cut down on verbosity:  s/Rusage:://, maybe s/HardPageFaults/HardFaults/. 

Would be nice to measure time spent in XPCOMGlueEnablePreload and expose that in our startup info, but that's better done in a followup.
Comment 8 User image Shawn Wilsher :sdwilsh 2011-05-18 14:51:50 PDT
(In reply to comment #7)
> I would use KB to be consistent with existing telemetry probes and the rest
> of Firefox. We don't use KiB elsewhere.
With the exception of about:cache, but that probably never went through UX review which is why it wasn't caught.  en-us uses KB (although I'm confused why we seem to be hard coding strings for a UI in C++ code here).
Comment 9 User image Mike Hommey [:glandium] 2011-05-18 22:43:41 PDT
(In reply to comment #6)
> >+  return *aResult ? NS_OK : NS_ERROR_FAILURE;
> 
> This should be NS_OK. It's never null. 

Yeah, looking at the histogram code it will actually crash (NULL deref) if a malloc fails, instead of returning NULL.
 
> For instance and the rest, we prefer explicit "if (NS_FAILED(rv)) return
> rv;" in new code.

That's sad, NS_ENSURE_SUCCESS has the advantage of being more concise *and* issuing a message when the unexpected happens on debug builds.

(In reply to comment #7)
> Comment on attachment 533262 [details] [diff] [review] [review]
> part 2 - Add telemetry samples for initial I/O counters on Windows in order
> to detect prefetch
> 
> >+struct jsval; // for nsITelemetry.h
> >+#include "nsITelemetry.h" // for nsITelemetry::HISTOGRAM_* consts
> 
> Yuck. Det up an enum in nsXULAppAPI.h  that is equivalent to values of
> nsITelemetry::HISTOGRAM_. Those constants aren't gonna change.

But new ones may be added... Need to add a comment to nsITelemetry to say to update the enum then...

> >+                     int(ioCounters.ReadTransferCount / 1024), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);
> 
> You max needs to be /1024 there. 10 * 1024 seems more reasonable to me.
> Otherwise the distribution into buckets is going to be rather narrow.

Even on an exponential histogram?

> Would be nice to measure time spent in XPCOMGlueEnablePreload and expose
> that in our startup info, but that's better done in a followup.

XPCOMGlueEnablePreload does nothing ;) XPCOMGlueStartup is the one.

Anyways, if that's not going to make it to Firefox 6, we can use better counters than the ones I put in there, like actual I/O count on linux instead of assuming some readahead size (which is likely variable anyways) and actual I/O count on windows, which would help make a real difference between cold and warm preload without prefetch.

(In reply to comment #8)
> (In reply to comment #7)
> > I would use KB to be consistent with existing telemetry probes and the rest
> > of Firefox. We don't use KiB elsewhere.
> With the exception of about:cache, but that probably never went through UX
> review which is why it wasn't caught.  en-us uses KB (although I'm confused
> why we seem to be hard coding strings for a UI in C++ code here).

They are more identifiers than UI strings.
Comment 10 User image Mike Hommey [:glandium] 2011-05-19 04:24:23 PDT
Created attachment 533593 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Refreshed with tglek's review. I'd still want bsmedberg to take a look
at the XRE addition.
Comment 11 User image Mike Hommey [:glandium] 2011-05-19 04:30:59 PDT
Created attachment 533596 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

I had forgotten the enum
Comment 12 User image Mike Hommey [:glandium] 2011-05-19 07:46:25 PDT
Created attachment 533645 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

With proper enum syntax
Comment 13 User image Mike Hommey [:glandium] 2011-05-19 08:56:07 PDT
Created attachment 533661 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

This is getting embarassing... this one actually works
Comment 14 User image (dormant account) 2011-05-19 09:28:41 PDT
> > For instance and the rest, we prefer explicit "if (NS_FAILED(rv)) return
> > rv;" in new code.
> 
> That's sad, NS_ENSURE_SUCCESS has the advantage of being more concise *and*
> issuing a message when the unexpected happens on debug builds.

I'll let Benjamin decide, it doesn't matter much to me.

> 
> But new ones may be added... Need to add a comment to nsITelemetry to say to
> update the enum then...

Yes
> 
> > >+                     int(ioCounters.ReadTransferCount / 1024), 1, 1000000, 10, nsITelemetry::HISTOGRAM_EXPONENTIAL);
> > 
> > You max needs to be /1024 there. 10 * 1024 seems more reasonable to me.
> > Otherwise the distribution into buckets is going to be rather narrow.
> 
> Even on an exponential histogram?

Yes, you trade off precision in common reports for max range, which in this case is likely to never be hit.
Comment 15 User image Mike Hommey [:glandium] 2011-05-20 04:52:38 PDT
Created attachment 533929 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch

I guess these buckets would be better. Note that 12 turns out to be better than 10 as a number of buckets, because there are 2 "reserved" buckets: one for 0 and another for max.
Comment 16 User image Benjamin Smedberg [:bsmedberg] 2011-05-20 11:12:21 PDT
Comment on attachment 533661 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

I don't think we need or want the separate HistogramTypes enum: just pass nsITelemetry::HISTOGRAM_EXPONENTIAL directly. sr=me with that change
Comment 17 User image Mike Hommey [:glandium] 2011-05-21 23:20:21 PDT
Created attachment 534276 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Maemo compiler doesn't like a comma at the end of an enum
Comment 19 User image Mike Hommey [:glandium] 2011-05-22 08:54:34 PDT
Backed out because of leak test bustage
http://hg.mozilla.org/mozilla-central/rev/aa83abd4fd01
Comment 20 User image Mike Hommey [:glandium] 2011-05-22 22:19:48 PDT
Created attachment 534369 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Without the enum
Comment 21 User image Mike Hommey [:glandium] 2011-05-22 22:20:40 PDT
Created attachment 534370 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.
Comment 22 User image Mike Hommey [:glandium] 2011-05-23 08:08:56 PDT
Created attachment 534441 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Removed remaining reference to the HistogramTypes enum
Comment 23 User image Mike Hommey [:glandium] 2011-06-16 22:52:35 PDT
Created attachment 539994 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Refreshed against bug 661574
Comment 24 User image Mike Hommey [:glandium] 2011-06-16 22:53:27 PDT
Created attachment 539995 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.

Refreshed against bug 661574
Comment 25 User image (dormant account) 2011-06-20 17:57:55 PDT
Comment on attachment 539995 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.

use EXPONENTIAL for the second histogram

Note You need to log in before you can comment on or make changes to this bug.