From 5c954c328732b943825091206d12ee9ea6382ed6 Mon Sep 17 00:00:00 2001 From: Taras Glek Date: Thu, 19 May 2011 19:33:05 -0700 Subject: [PATCH] Bug 657709: Cleanup histogram API r=mrbkap --HG-- rename : xpcom/base/nsTelemetry.cpp => xpcom/base/Telemetry.cpp --- toolkit/components/telemetry/TelemetryPing.js | 8 +-- .../tests/unit/test_TelemetryPing.js | 2 +- xpcom/base/Makefile.in | 2 +- xpcom/base/{nsTelemetry.cpp => Telemetry.cpp} | 57 +++++++++++-------- xpcom/base/nsITelemetry.idl | 21 ++++--- xpcom/tests/unit/test_nsITelemetry.js | 30 ++++++++-- 6 files changed, 76 insertions(+), 44 deletions(-) rename xpcom/base/{nsTelemetry.cpp => Telemetry.cpp} (85%) diff --git a/toolkit/components/telemetry/TelemetryPing.js b/toolkit/components/telemetry/TelemetryPing.js index d3bfa256ff8..d60ae69affc 100644 --- a/toolkit/components/telemetry/TelemetryPing.js +++ b/toolkit/components/telemetry/TelemetryPing.js @@ -124,7 +124,7 @@ function getMetadata(reason) { OS: ai.OS, XPCOMABI: ai.XPCOMABI, ID: ai.ID, - vesion: ai.version, + version: ai.version, name: ai.name, appBuildID: ai.appBuildID, platformBuildID: ai.platformBuildID, @@ -163,7 +163,7 @@ TelemetryPing.prototype = { let name = "Memory:" + mr.path + " (KB)"; let h = this._histograms[name]; if (!h) { - h = Telemetry.newExponentialHistogram(name, specs[0], specs[1], specs[2]); + h = Telemetry.newHistogram(name, specs[0], specs[1], specs[2], Telemetry.HISTOGRAM_EXPONENTIAL); this._histograms[name] = h; } let v = Math.floor(mr.memoryUsed / 1024); @@ -192,8 +192,8 @@ TelemetryPing.prototype = { const TELEMETRY_PING = "telemetry.ping (ms)"; const TELEMETRY_SUCCESS = "telemetry.success (No, Yes)"; - let hping = Telemetry.newExponentialHistogram(TELEMETRY_PING, 1, 3000, 10); - let hsuccess = Telemetry.newLinearHistogram(TELEMETRY_SUCCESS, 1, 2, 3); + let hping = Telemetry.newHistogram(TELEMETRY_PING, 1, 3000, 10, Telemetry.HISTOGRAM_EXPONENTIAL); + let hsuccess = Telemetry.newHistogram(TELEMETRY_SUCCESS, 1, 2, 3, Telemetry.HISTOGRAM_LINEAR); let url = server + this._path; let request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js index a037f945653..e3ee1494cbc 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ -65,7 +65,7 @@ function checkHistograms(request, response) { OS: "XPCShell", XPCOMABI: "noarch-spidermonkey", ID: "xpcshell@tests.mozilla.org", - vesion: "1", + version: "1", name: "XPCShell", appBuildID: "2007010101", platformBuildID: "2007010101" diff --git a/xpcom/base/Makefile.in b/xpcom/base/Makefile.in index 4e3ca15c969..a1aa1bc91d0 100644 --- a/xpcom/base/Makefile.in +++ b/xpcom/base/Makefile.in @@ -70,7 +70,7 @@ CPPSRCS = \ nsStackWalk.cpp \ nsMemoryReporterManager.cpp \ FunctionTimer.cpp \ - nsTelemetry.cpp \ + Telemetry.cpp \ $(NULL) ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)) diff --git a/xpcom/base/nsTelemetry.cpp b/xpcom/base/Telemetry.cpp similarity index 85% rename from xpcom/base/nsTelemetry.cpp rename to xpcom/base/Telemetry.cpp index d65977a7cdc..eaf57d3a311 100644 --- a/xpcom/base/nsTelemetry.cpp +++ b/xpcom/base/Telemetry.cpp @@ -48,22 +48,23 @@ #include "nsStringGlue.h" #include "nsITelemetry.h" +namespace { + using namespace base; -using namespace mozilla; class Telemetry : public nsITelemetry { NS_DECL_ISUPPORTS NS_DECL_NSITELEMETRY - public: +public: static Telemetry* GetSingleton(); }; -// A static initializer to initialize histogram collection -static StatisticsRecorder gStatisticsRecorder; +// A initializer to initialize histogram collection +StatisticsRecorder gStatisticsRecorder; -static bool +bool FillRanges(JSContext *cx, JSObject *array, Histogram *h) { for (size_t i = 0;i < h->bucket_count();i++) { @@ -73,7 +74,7 @@ FillRanges(JSContext *cx, JSObject *array, Histogram *h) return true; } -static JSBool +JSBool ReflectHistogramSnapshot(JSContext *cx, JSObject *obj, Histogram *h) { Histogram::SampleSet ss; @@ -101,7 +102,7 @@ ReflectHistogramSnapshot(JSContext *cx, JSObject *obj, Histogram *h) return JS_TRUE; } -static JSBool +JSBool JSHistogram_Add(JSContext *cx, uintN argc, jsval *vp) { jsval *argv = JS_ARGV(cx, vp); @@ -116,7 +117,7 @@ JSHistogram_Add(JSContext *cx, uintN argc, jsval *vp) return JS_TRUE; } -static JSBool +JSBool JSHistogram_Snapshot(JSContext *cx, uintN argc, jsval *vp) { JSObject *obj = JS_THIS_OBJECT(cx, vp); @@ -128,7 +129,7 @@ JSHistogram_Snapshot(JSContext *cx, uintN argc, jsval *vp) return ReflectHistogramSnapshot(cx, snapshot, h); } -static nsresult +nsresult WrapAndReturnHistogram(Histogram *h, JSContext *cx, jsval *ret) { static JSClass JSHistogram_class = { @@ -149,16 +150,24 @@ WrapAndReturnHistogram(Histogram *h, JSContext *cx, jsval *ret) } NS_IMETHODIMP -Telemetry::NewExponentialHistogram(const nsACString &name, PRInt32 min, PRInt32 max, PRUint32 size, JSContext *cx, jsval *ret) +Telemetry::NewHistogram(const nsACString &name, PRUint32 min, PRUint32 max, PRUint32 bucket_count, PRUint32 histogram_type, JSContext *cx, jsval *ret) { - Histogram *h = base::Histogram::FactoryGet(name.BeginReading(), min, max, size, base::Histogram::kNoFlags); - return WrapAndReturnHistogram(h, cx, ret); -} + // Sanity checks on histogram parameters. + if (min < 1) + return NS_ERROR_ILLEGAL_VALUE; -NS_IMETHODIMP -Telemetry::NewLinearHistogram(const nsACString &name, PRInt32 min, PRInt32 max, PRUint32 size, JSContext *cx, jsval *ret) -{ - Histogram *h = base::LinearHistogram::FactoryGet(name.BeginReading(), min, max, size, base::Histogram::kNoFlags); + if (min >= max) + return NS_ERROR_ILLEGAL_VALUE; + + if (bucket_count <= 2) + return NS_ERROR_ILLEGAL_VALUE; + + 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); + } return WrapAndReturnHistogram(h, cx, ret); } @@ -187,9 +196,9 @@ Telemetry::GetHistogramSnapshots(JSContext *cx, jsval *ret) NS_IMPL_THREADSAFE_ISUPPORTS1(Telemetry, nsITelemetry) -static Telemetry *gJarHandler = nsnull; +Telemetry *gJarHandler = nsnull; -static void ShutdownTelemetry() +void ShutdownTelemetry() { NS_IF_RELEASE(gJarHandler); } @@ -207,20 +216,20 @@ Telemetry* Telemetry::GetSingleton() NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(Telemetry, Telemetry::GetSingleton) #define NS_TELEMETRY_CID \ - {0xf880b792, 0xe6cd, 0x46e7, {0x9c, 0x22, 0x3e, 0x12, 0xc3, 0x8b, 0xc6, 0xca}} + {0xaea477f2, 0xb3a2, 0x469c, {0xaa, 0x29, 0x0a, 0x82, 0xd1, 0x32, 0xb8, 0x29}} NS_DEFINE_NAMED_CID(NS_TELEMETRY_CID); -static const mozilla::Module::CIDEntry kTelemetryCIDs[] = { +const mozilla::Module::CIDEntry kTelemetryCIDs[] = { { &kNS_TELEMETRY_CID, false, NULL, TelemetryConstructor }, { NULL } }; -static const mozilla::Module::ContractIDEntry kTelemetryContracts[] = { +const mozilla::Module::ContractIDEntry kTelemetryContracts[] = { { "@mozilla.org/base/telemetry;1", &kNS_TELEMETRY_CID }, { NULL } }; -static const mozilla::Module kTelemetryModule = { +const mozilla::Module kTelemetryModule = { mozilla::Module::kVersion, kTelemetryCIDs, kTelemetryContracts, @@ -230,4 +239,6 @@ static const mozilla::Module kTelemetryModule = { ShutdownTelemetry, }; +} // anonymous namespace + NSMODULE_DEFN(nsTelemetryModule) = &kTelemetryModule; diff --git a/xpcom/base/nsITelemetry.idl b/xpcom/base/nsITelemetry.idl index 2c6bb761441..e9b7c0717ae 100644 --- a/xpcom/base/nsITelemetry.idl +++ b/xpcom/base/nsITelemetry.idl @@ -38,16 +38,24 @@ #include "nsISupports.idl" -[scriptable, uuid(5c9afdb5-0532-47f3-be31-79e13a6db642)] +[scriptable, uuid(29464d3d-f838-4afb-a737-319fe0c6cc04)] interface nsITelemetry : nsISupports { + /** + * Histogram types: + * HISTOGRAM_EXPONENTIAL - buckets increase exponentially + * HISTOGRAM_LINEAR - buckets increase linearly + */ + const unsigned long HISTOGRAM_EXPONENTIAL = 0; + const unsigned long HISTOGRAM_LINEAR = 1; + /* * An object containing a snapshot from all of the currently registered histograms. * { name1: {data1}, name2:{data2}...} * where data is consists of the following properties: * min - Minimal bucket size * max - Maximum bucket size - * histogram_type - 0:Exponential, 1:Linear + * histogram_type - HISTOGRAM_EXPONENTIAL or HISTOGRAM_LINEAR * counts - array representing contents of the buckets in the histogram * ranges - an array with calculated bucket sizes * sum - sum of the bucket contents @@ -61,16 +69,11 @@ interface nsITelemetry : nsISupports * @param min - Minimal bucket size * @param max - Maximum bucket size * @param bucket_count - number of buckets in the histogram. + * @param type - HISTOGRAM_EXPONENTIAL or HISTOGRAM_LINEAR * The returned object has the following functions: * add(int) - Adds an int value to the appropriate bucket * snapshot() - Returns a snapshot of the histogram with the same data fields as in histogramSnapshots() */ [implicit_jscontext] - jsval newExponentialHistogram(in ACString name, in PRInt32 min, in PRInt32 max, in PRUint32 bucket_count); - - /* - Same as newExponentialHistogram, but for linear histograms - */ - [implicit_jscontext] - jsval newLinearHistogram(in ACString name, in PRInt32 min, in PRInt32 max, in PRUint32 bucket_count); + jsval newHistogram(in ACString name, in PRUint32 min, in PRUint32 max, in PRUint32 bucket_count, in unsigned long histogram_type); }; diff --git a/xpcom/tests/unit/test_nsITelemetry.js b/xpcom/tests/unit/test_nsITelemetry.js index 8de4edaf7be..cbcdb7ba4fc 100644 --- a/xpcom/tests/unit/test_nsITelemetry.js +++ b/xpcom/tests/unit/test_nsITelemetry.js @@ -6,8 +6,8 @@ const Ci = Components.interfaces; const Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry); -function test_histogram(histogram_constructor, name, min, max, bucket_count) { - var h = histogram_constructor(name, min, max, bucket_count); +function test_histogram(histogram_type, name, min, max, bucket_count) { + var h = Telemetry.newHistogram(name, min, max, bucket_count, histogram_type); var r = h.snapshot().ranges; var sum = 0; @@ -25,14 +25,32 @@ function test_histogram(histogram_constructor, name, min, max, bucket_count) { } var hgrams = Telemetry.histogramSnapshots gh = hgrams[name] - do_check_eq(gh.histogram_type, - histogram_constructor == Telemetry.newExponentialHistogram ? 0 : 1); + do_check_eq(gh.histogram_type, histogram_type); + do_check_eq(gh.min, min) do_check_eq(gh.max, max) } +function expect_fail(f) { + let failed = false; + try { + f(); + failed = false; + } catch (e) { + failed = true; + } + do_check_true(failed); +} + function run_test() { - test_histogram(Telemetry.newExponentialHistogram, "test::Exponential", 1, 10000, 10); - test_histogram(Telemetry.newLinearHistogram, "test::Linear", 1, 10000, 10); + let kinds = [Telemetry.HISTOGRAM_EXPONENTIAL, Telemetry.HISTOGRAM_LINEAR] + for each (let histogram_type in kinds) { + let [min, max, bucket_count] = [1, 10000, 10] + test_histogram(histogram_type, "test::"+histogram_type, min, max, bucket_count); + + const nh = Telemetry.newHistogram; + expect_fail(function () nh("test::min", 0, max, bucket_count, histogram_type)); + expect_fail(function () nh("test::bucket_count", min, max, 1, histogram_type)); + } }