Closed Bug 808557 Opened 8 years ago Closed 8 years ago

add telemetry probe for measuring the impact of UIC_startSearch synchronous query

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 791776 suggests that the synchronous query of UIC_startSearch can be damaging. We should investigate exactly how much.
Assignee: nobody → dteller
Attachment #678300 - Flags: review?(mak77)
Comment on attachment 678300 [details] [diff] [review]
telemetry to measure the impact of UIC_startSearch synchronous query

Review of attachment 678300 [details] [diff] [review]:
-----------------------------------------------------------------

this likely doesn't work, but the approach is fine.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +1394,5 @@
>      if (lastSlashIndex == -1) {
>        var hasDomainResult = false;
>        var domain, untrimmedDomain;
> +      let refObj = {};
> +      TelemetryStopwatch.start(DOMAIN_QUERY_TELEMETRY, refObj);

shouldn't you import the module somewhere?
As it is this just breaks the functionality... I expect tests to fail!

The refObj doesn't look needed since this is synchronous, there can't just be concurrent calls, so it can be omitted.

@@ +1400,5 @@
> +        // Execute the query synchronously.
> +        // This is by design, to avoid race conditions between the
> +        // user typing and the connection searching for the result.
> +        // Telemetry (bug 808557) will tell us if we need to change that
> +        // (bug 791776).

The latter part of the comment feels redundant, we use bugzilla to track bugs, not the code :)

::: toolkit/components/telemetry/Histograms.json
@@ +1804,5 @@
>      "high": "500",
>      "n_buckets": 10,
>      "description": "PLACES: Time for first autocomplete result if > 50ms (ms)"
>    },
> +  "PLACES_AUTOCOMPLETE_URLINLINECOMPLETE_DOMAIN_QUERY_TIME_MS": {

nit: just to shrink this a bit you may just use _URLINLINE_ and avoid to repeat COMPLETE (it's already in AUTOCOMPLETE)
Attachment #678300 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 678300 [details] [diff] [review]
> telemetry to measure the impact of UIC_startSearch synchronous query
> 
> Review of attachment 678300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this likely doesn't work, but the approach is fine.
> 
> ::: toolkit/components/places/nsPlacesAutoComplete.js
> @@ +1394,5 @@
> >      if (lastSlashIndex == -1) {
> >        var hasDomainResult = false;
> >        var domain, untrimmedDomain;
> > +      let refObj = {};
> > +      TelemetryStopwatch.start(DOMAIN_QUERY_TELEMETRY, refObj);
> 
> shouldn't you import the module somewhere?
> As it is this just breaks the functionality... I expect tests to fail!
> 
> The refObj doesn't look needed since this is synchronous, there can't just
> be concurrent calls, so it can be omitted.
> 
> @@ +1400,5 @@
> > +        // Execute the query synchronously.
> > +        // This is by design, to avoid race conditions between the
> > +        // user typing and the connection searching for the result.
> > +        // Telemetry (bug 808557) will tell us if we need to change that
> > +        // (bug 791776).
> 
> The latter part of the comment feels redundant, we use bugzilla to track
> bugs, not the code :)
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +1804,5 @@
> >      "high": "500",
> >      "n_buckets": 10,
> >      "description": "PLACES: Time for first autocomplete result if > 50ms (ms)"
> >    },
> > +  "PLACES_AUTOCOMPLETE_URLINLINECOMPLETE_DOMAIN_QUERY_TIME_MS": {
> 
> nit: just to shrink this a bit you may just use _URLINLINE_ and avoid to
> repeat COMPLETE (it's already in AUTOCOMPLETE)
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 678300 [details] [diff] [review]
> telemetry to measure the impact of UIC_startSearch synchronous query
> 
> Review of attachment 678300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this likely doesn't work, but the approach is fine.
> 
> ::: toolkit/components/places/nsPlacesAutoComplete.js
> @@ +1394,5 @@
> >      if (lastSlashIndex == -1) {
> >        var hasDomainResult = false;
> >        var domain, untrimmedDomain;
> > +      let refObj = {};
> > +      TelemetryStopwatch.start(DOMAIN_QUERY_TELEMETRY, refObj);
> 
> shouldn't you import the module somewhere?
> As it is this just breaks the functionality... I expect tests to fail!

Oops.

> 
> The refObj doesn't look needed since this is synchronous, there can't just
> be concurrent calls, so it can be omitted.

Right. I may have my mind a little too much asynch-oriented these days :)

> 
> @@ +1400,5 @@
> > +        // Execute the query synchronously.
> > +        // This is by design, to avoid race conditions between the
> > +        // user typing and the connection searching for the result.
> > +        // Telemetry (bug 808557) will tell us if we need to change that
> > +        // (bug 791776).
> 
> The latter part of the comment feels redundant, we use bugzilla to track
> bugs, not the code :)

Ok, if you insist :)

> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +1804,5 @@
> >      "high": "500",
> >      "n_buckets": 10,
> >      "description": "PLACES: Time for first autocomplete result if > 50ms (ms)"
> >    },
> > +  "PLACES_AUTOCOMPLETE_URLINLINECOMPLETE_DOMAIN_QUERY_TIME_MS": {
> 
> nit: just to shrink this a bit you may just use _URLINLINE_ and avoid to
> repeat COMPLETE (it's already in AUTOCOMPLETE)

Done, thanks.
OS: Mac OS X → All
Hardware: x86 → All
Attachment #682864 - Flags: review?(mak77)
Comment on attachment 682864 [details] [diff] [review]
telemetry to measure the impact of UIC_startSearch synchronous query, v2

Review of attachment 682864 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you
Attachment #682864 - Flags: review?(mak77) → review+
Thanks for the quick review, btw.
https://hg.mozilla.org/mozilla-central/rev/dfa9f6c4c608
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.