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

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.

This exposes an API similar to that of bug 657709 (which it depends upon)

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

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

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

Refreshed with tglek's review. I'd still want bsmedberg to take a look at the XRE addition.

I had forgotten the enum

With proper enum syntax

This is getting embarassing... this one actually works

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

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

Maemo compiler doesn't like a comma at the end of an enum

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

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

Without the enum

Removed remaining reference to the HistogramTypes enum

Refreshed against bug 661574

Refreshed against bug 661574

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

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

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