The default bug view has changed. See this FAQ.

Instrument nsSearchService initialization with Telemetry

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Search
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
Firefox 21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Before proceeding to further refactorings in nsSearchService, we should add some Telemetry probes.
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.
Assignee: nobody → dteller
Attachment #699849 - Flags: feedback?(gavin.sharp)
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).
Attachment #699849 - Flags: feedback?(gavin.sharp) → feedback+
Created attachment 700264 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization
Attachment #699849 - Attachment is obsolete: true
Attachment #700264 - Flags: review?(gavin.sharp)
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.
Attachment #700264 - Flags: review?(gavin.sharp) → review+
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.
Created attachment 702754 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization, v2

Moving the patch before bug 828223.
Attachment #700264 - Attachment is obsolete: true
Attachment #702754 - Flags: review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=05aba2d53945
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca602d98d14
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eca602d98d14
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.