808557 - add telemetry probe for measuring the impact of UIC_startSearch synchronous query

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Bug 791776 suggests that the synchronous query of UIC_startSearch can be damaging. We should investigate exactly how much.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: telemetry to measure the impact of UIC_startSearch synchronous query

Comment 2

[Marco Bonardo [:mak]]

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)

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(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)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: telemetry to measure the impact of UIC_startSearch synchronous query, v2

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=3afd0ec8864b

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Thanks for the quick review, btw.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa9f6c4c608

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dfa9f6c4c608