Telemetry probe for preloading

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 13 obsolete attachments)

1.77 KB, patch
(dormant account)
: review+
Details | Diff | Splinter Review
6.56 KB, patch
(dormant account)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
This is about
https://bug627591.bugzilla.mozilla.org/attachment.cgi?id=530687
for bug 632404.
(Reporter)

Comment 1

6 years ago
Created attachment 532980 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE
Attachment #532980 - Flags: review?(benjamin)
(Reporter)

Comment 2

6 years ago
Created attachment 532984 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch
Attachment #532984 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #532980 - Flags: review?(benjamin)

Comment 3

6 years ago
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.
Attachment #532984 - Flags: review?(tglek) → review-
(Reporter)

Updated

6 years ago
Depends on: 657709
(Reporter)

Updated

6 years ago
Summary: Telemetry probe for windows preloading decision → Telemetry probe for preloading
(Reporter)

Comment 4

6 years ago
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)
Attachment #533223 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #532980 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #533223 - Flags: review?(benjamin)
(Reporter)

Comment 5

6 years ago
Created attachment 533262 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters on Windows in order to detect prefetch
Attachment #533262 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #532984 - Attachment is obsolete: true

Comment 6

6 years ago
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.
Attachment #533223 - Flags: review?(tglek) → review+

Comment 7

6 years ago
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.
Attachment #533262 - Flags: review?(tglek) → review+
(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).
(Reporter)

Comment 9

6 years ago
(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.
(Reporter)

Comment 10

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #533223 - Attachment is obsolete: true
Attachment #533223 - Flags: review?(benjamin)
(Reporter)

Updated

6 years ago
Attachment #533593 - Flags: superreview?(benjamin)
(Reporter)

Comment 11

6 years ago
Created attachment 533596 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

I had forgotten the enum
(Reporter)

Updated

6 years ago
Attachment #533593 - Attachment is obsolete: true
Attachment #533593 - Flags: superreview?(benjamin)
(Reporter)

Updated

6 years ago
Attachment #533596 - Flags: superreview?(benjamin)
(Reporter)

Comment 12

6 years ago
Created attachment 533645 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

With proper enum syntax
(Reporter)

Updated

6 years ago
Attachment #533596 - Attachment is obsolete: true
Attachment #533596 - Flags: superreview?(benjamin)
(Reporter)

Updated

6 years ago
Attachment #533645 - Flags: superreview?(benjamin)
(Reporter)

Comment 13

6 years ago
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
(Reporter)

Updated

6 years ago
Attachment #533645 - Attachment is obsolete: true
Attachment #533645 - Flags: superreview?(benjamin)
(Reporter)

Updated

6 years ago
Attachment #533661 - Flags: superreview?(benjamin)

Comment 14

6 years ago
> > 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.
(Reporter)

Comment 15

6 years ago
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.
Attachment #533929 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #533262 - Attachment is obsolete: true

Updated

6 years ago
Attachment #533929 - Flags: review?(tglek) → review+
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
Attachment #533661 - Flags: superreview?(benjamin) → superreview+
(Reporter)

Comment 17

6 years ago
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
(Reporter)

Updated

6 years ago
Attachment #533661 - Attachment is obsolete: true
(Reporter)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/df31a6f3e470
http://hg.mozilla.org/mozilla-central/rev/35f142a55f50
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Reporter)

Updated

6 years ago
Depends on: 658854
(Reporter)

Comment 19

6 years ago
Backed out because of leak test bustage
http://hg.mozilla.org/mozilla-central/rev/aa83abd4fd01
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 20

6 years ago
Created attachment 534369 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Without the enum
Attachment #534369 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #534276 - Attachment is obsolete: true
(Reporter)

Comment 21

6 years ago
Created attachment 534370 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.
Attachment #534370 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #533929 - Attachment is obsolete: true
(Reporter)

Comment 22

6 years ago
Created attachment 534441 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Removed remaining reference to the HistogramTypes enum
Attachment #534441 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #534369 - Attachment is obsolete: true
Attachment #534369 - Flags: review?(tglek)

Updated

6 years ago
Attachment #534441 - Flags: review?(tglek) → review+

Updated

6 years ago
Attachment #534370 - Flags: review?(tglek) → review+
(Reporter)

Updated

6 years ago
Blocks: 659028

Updated

6 years ago
Blocks: 659396
(Reporter)

Updated

6 years ago
Depends on: 661574
(Reporter)

Comment 23

6 years ago
Created attachment 539994 [details] [diff] [review]
part 1 - Expose a function to add telemetry samples in XRE.

Refreshed against bug 661574
Attachment #539994 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #534441 - Attachment is obsolete: true
(Reporter)

Comment 24

6 years ago
Created attachment 539995 [details] [diff] [review]
part 2 - Add telemetry samples for initial I/O counters.

Refreshed against bug 661574
Attachment #539995 - Flags: review?(tglek)
(Reporter)

Updated

6 years ago
Attachment #534370 - Attachment is obsolete: true

Updated

6 years ago
Attachment #539994 - Flags: review?(tglek) → review+

Comment 25

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

use EXPONENTIAL for the second histogram
Attachment #539995 - Flags: review?(tglek) → review+
(Reporter)

Comment 26

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8eead40fb58c
http://hg.mozilla.org/integration/mozilla-inbound/rev/59a1a0103096
Whiteboard: [inbound]
Target Milestone: mozilla6 → mozilla7
http://hg.mozilla.org/mozilla-central/rev/8eead40fb58c
http://hg.mozilla.org/mozilla-central/rev/59a1a0103096
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.