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
:
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 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 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 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 (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 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 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 (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 (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 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 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 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 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 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 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 (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 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 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 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 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 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 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 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 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 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 (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.