Closed
Bug 657297
Opened 14 years ago
Closed 14 years ago
Telemetry probe for preloading
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: glandium, Unassigned)
References
Details
Attachments
(2 files, 13 obsolete files)
1.77 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
6.56 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
This is about
https://bug627591.bugzilla.mozilla.org/attachment.cgi?id=530687
for bug 632404.
Reporter | ||
Comment 1•14 years ago
|
||
Attachment #532980 -
Flags: review?(benjamin)
Reporter | ||
Comment 2•14 years ago
|
||
Attachment #532984 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #532980 -
Flags: review?(benjamin)
Comment 3•14 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•14 years ago
|
Summary: Telemetry probe for windows preloading decision → Telemetry probe for preloading
Reporter | ||
Comment 4•14 years ago
|
||
This exposes an API similar to that of bug 657709 (which it depends upon)
Attachment #533223 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #532980 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Attachment #533223 -
Flags: review?(benjamin)
Reporter | ||
Comment 5•14 years ago
|
||
Attachment #533262 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #532984 -
Attachment is obsolete: true
Comment 6•14 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•14 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+
Comment 8•14 years ago
|
||
(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•14 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•14 years ago
|
||
Refreshed with tglek's review. I'd still want bsmedberg to take a look
at the XRE addition.
Reporter | ||
Updated•14 years ago
|
Attachment #533223 -
Attachment is obsolete: true
Attachment #533223 -
Flags: review?(benjamin)
Reporter | ||
Updated•14 years ago
|
Attachment #533593 -
Flags: superreview?(benjamin)
Reporter | ||
Comment 11•14 years ago
|
||
I had forgotten the enum
Reporter | ||
Updated•14 years ago
|
Attachment #533593 -
Attachment is obsolete: true
Attachment #533593 -
Flags: superreview?(benjamin)
Reporter | ||
Updated•14 years ago
|
Attachment #533596 -
Flags: superreview?(benjamin)
Reporter | ||
Comment 12•14 years ago
|
||
With proper enum syntax
Reporter | ||
Updated•14 years ago
|
Attachment #533596 -
Attachment is obsolete: true
Attachment #533596 -
Flags: superreview?(benjamin)
Reporter | ||
Updated•14 years ago
|
Attachment #533645 -
Flags: superreview?(benjamin)
Reporter | ||
Comment 13•14 years ago
|
||
This is getting embarassing... this one actually works
Reporter | ||
Updated•14 years ago
|
Attachment #533645 -
Attachment is obsolete: true
Attachment #533645 -
Flags: superreview?(benjamin)
Reporter | ||
Updated•14 years ago
|
Attachment #533661 -
Flags: superreview?(benjamin)
Comment 14•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #533262 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #533929 -
Flags: review?(tglek) → review+
Comment 16•14 years ago
|
||
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•14 years ago
|
||
Maemo compiler doesn't like a comma at the end of an enum
Reporter | ||
Updated•14 years ago
|
Attachment #533661 -
Attachment is obsolete: true
Reporter | ||
Comment 18•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/df31a6f3e470
http://hg.mozilla.org/mozilla-central/rev/35f142a55f50
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Reporter | ||
Comment 19•14 years ago
|
||
Backed out because of leak test bustage
http://hg.mozilla.org/mozilla-central/rev/aa83abd4fd01
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•14 years ago
|
Attachment #534276 -
Attachment is obsolete: true
Reporter | ||
Comment 21•14 years ago
|
||
Attachment #534370 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #533929 -
Attachment is obsolete: true
Reporter | ||
Comment 22•14 years ago
|
||
Removed remaining reference to the HistogramTypes enum
Attachment #534441 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #534369 -
Attachment is obsolete: true
Attachment #534369 -
Flags: review?(tglek)
Updated•14 years ago
|
Attachment #534441 -
Flags: review?(tglek) → review+
Updated•14 years ago
|
Attachment #534370 -
Flags: review?(tglek) → review+
Reporter | ||
Comment 23•14 years ago
|
||
Refreshed against bug 661574
Attachment #539994 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #534441 -
Attachment is obsolete: true
Reporter | ||
Comment 24•14 years ago
|
||
Refreshed against bug 661574
Attachment #539995 -
Flags: review?(tglek)
Reporter | ||
Updated•14 years ago
|
Attachment #534370 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #539994 -
Flags: review?(tglek) → review+
Comment 25•14 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•14 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
Comment 27•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/8eead40fb58c
http://hg.mozilla.org/mozilla-central/rev/59a1a0103096
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•