Last Comment Bug 828228 - Instrument nsSearchService initialization with Telemetry
: Instrument nsSearchService initialization with Telemetry
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 21
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on:
Blocks: 706523
  Show dependency treegraph
 
Reported: 2013-01-09 03:44 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2013-01-17 02:54 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First prototype (5.72 KB, patch)
2013-01-09 08:26 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: feedback+
Details | Diff | Splinter Review
Some telemetry measurements for nsSearchService initialization (4.99 KB, patch)
2013-01-10 02:34 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: review+
Details | Diff | Splinter Review
Some telemetry measurements for nsSearchService initialization, v2 (5.08 KB, patch)
2013-01-16 03:27 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-09 03:44:38 PST
Before proceeding to further refactorings in nsSearchService, we should add some Telemetry probes.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-09 08:26:13 PST
Created attachment 699849 [details] [diff] [review]
First prototype

As I'm not sure exactly what we want to measure, let's start with a first prototype.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-09 09:51:57 PST
Comment on attachment 699849 [details] [diff] [review]
First prototype

Seems fine to me. I can't think of other things to measure offhand, but I haven't thought of it much.

The descriptions in the histogram definitions should be unique :)

One other nit: I don't like using constants for short strings, like TELEMETRY_SERVICE_INIT/TELEMETRY_BUILD_CACHE - it's a level of indirection that makes the code harder to read/grep, and doesn't really serve a purpose (typos would be caught otherwise, and we don't need to change these values often).
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-10 02:34:35 PST
Created attachment 700264 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-15 12:57:21 PST
Comment on attachment 700264 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>@@ -2639,16 +2642,17 @@ SearchService.prototype = {
>         null,
>         function onError(e) {
>           LOG("_buildCache: failure during writeAtomic: " + e);
>         }
>       );
>     } catch (ex) {
>       LOG("_buildCache: Could not write to cache file: " + ex);
>     }
>+    TelemetryStopwatch.finish("SEARCH_SERVICE_BUILD_CACHE_MS");

This doesn't actually measure writing the file to disk. Should you put this stop() in the writeAtomic callback instead? Depends on what the intent is I guess.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-16 03:26:04 PST
I assumed that we didn't need that measure, because it's not on the critical path. We can always add a further measure later, I guess.
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-16 03:27:51 PST
Created attachment 702754 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization, v2

Moving the patch before bug 828223.
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-16 08:00:18 PST
Try: https://tbpl.mozilla.org/?tree=Try&rev=05aba2d53945
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-01-16 15:05:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca602d98d14
Comment 9 Ed Morley [:emorley] 2013-01-17 02:54:57 PST
https://hg.mozilla.org/mozilla-central/rev/eca602d98d14

Note You need to log in before you can comment on or make changes to this bug.