657297 - Telemetry probe for preloading

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

This is about
https://bug627591.bugzilla.mozilla.org/attachment.cgi?id=530687
for bug 632404.

Comment 1

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch

Comment 3

[(dormant account)]

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]]

Attachment: 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]]

Attachment: part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch

Comment 6

[(dormant account)]

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)]

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]

(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]]

(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]]

Attachment: 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]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

I had forgotten the enum

Comment 12

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

With proper enum syntax

Comment 13

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

This is getting embarassing... this one actually works

Comment 14

[(dormant account)]

> > 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]]

Attachment: 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]

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]]

Attachment: 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 18

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/df31a6f3e470
http://hg.mozilla.org/mozilla-central/rev/35f142a55f50

Comment 19

[Mike Hommey [:glandium]]

Backed out because of leak test bustage
http://hg.mozilla.org/mozilla-central/rev/aa83abd4fd01

Comment 20

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

Without the enum

Comment 21

[Mike Hommey [:glandium]]

Attachment: part 2 - Add telemetry samples for initial I/O counters.

Comment 22

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

Removed remaining reference to the HistogramTypes enum

Comment 23

[Mike Hommey [:glandium]]

Attachment: part 1 - Expose a function to add telemetry samples in XRE.

Refreshed against bug 661574

Comment 24

[Mike Hommey [:glandium]]

Attachment: part 2 - Add telemetry samples for initial I/O counters.

Refreshed against bug 661574

Comment 25

[(dormant account)]

Comment on attachment 539995 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.

use EXPONENTIAL for the second histogram

Comment 26

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8eead40fb58c
http://hg.mozilla.org/integration/mozilla-inbound/rev/59a1a0103096

Comment 27

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/8eead40fb58c
http://hg.mozilla.org/mozilla-central/rev/59a1a0103096