1388331 - Use more precise criteria for stopping auto-complete searches

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement)

Description

[Simon Lindholm]

A common thing right now in UnifiedComplete.js is this:

- The user types a few character into the location bar
- The characters give rise to a heuristic autofill result
- A MATCH_BOUNDARY_ANYWHERE search is done, as an SQL query with LIMIT 10
- The query finds 10 results, but one of them is skipped, because it matches the heuristic result
- Hence _counts[MATCHTYPE.GENERAL] is only 9, so we do a pointless MATCH_ANYWHERE fallback search

It's pretty harmless, but still worth fixing the check in the last step I guess.

Comment 1

[Simon Lindholm]

Attachment: Use more precise criteria for stopping auto-complete searches

Another option would be to just change this._counts[MATCHTYPE.GENERAL] to this._counts[MATCHTYPE.GENERAL] + this._counts[MATCHTYPE.HEURISTIC] in the affected conditions

Comment 2

[:Paolo Amadini]

Thank you very much for the patch! I'm not sure I'll have time to look at it this week, even if the patch is short, because I'm not too familiar with this code yet. In the worst case it will be reviewed by Marco when he is back.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8894847 [details] [diff] [review]
Use more precise criteria for stopping auto-complete searches

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

So, the fact is we don't know

::: toolkit/components/places/UnifiedComplete.js
@@ +1732,1 @@
>        throw StopIteration;

heads-up: this will likely need an unbitrot, today is landing a patch removing StopIteration.

@@ +1866,5 @@
> +  _wantMoreMatches(type) {
> +    if (!this._buckets)
> +      return true;
> +    let index = 0;
> +    for (let bucket of this._buckets) {

looping through buckets every time looks more expensive than the option in comment 1, so I'd prefer that approach.

I understand this is more precise and would save some fetch calls, but with bug 1320301 we should have a quicker cancelation of the running queries, so I'm not too much concerned.

Comment 4

[Marco Bonardo [:mak]]

Please ignore the "So, the fact is we don't know" comment, it was just a phrase I never finished and I changed my mind a few seconds later, but forgot to delete it.

Comment 5

[Simon Lindholm]

> heads-up: this will likely need an unbitrot, today is landing a patch removing StopIteration.

Wonderful!

> I understand this is more precise and would save some fetch calls, but with bug 1320301 we should have a quicker cancelation of the running queries, so I'm not too much concerned.

Even more wonderful! So yeah, this isn't very much of an optimization, more correctness for sanity's sake.

> looping through buckets every time looks more expensive than the option in comment 1, so I'd prefer that approach.

OK. (I guess I'm spoiled by C++, where that loop would take less than a microsecond...) I'll post an updated patch.

Comment 6

[Simon Lindholm]

Attachment: Use more precise criteria for stopping auto-complete searches

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8900766 [details] [diff] [review]
Use more precise criteria for stopping auto-complete searches

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

::: toolkit/components/places/UnifiedComplete.js
@@ +1066,5 @@
>      // If we do not have enough results, and our match type is
>      // MATCH_BOUNDARY_ANYWHERE, search again with MATCH_ANYWHERE to get more
>      // results.
>      if (this._matchBehavior == MATCH_BOUNDARY_ANYWHERE &&
> +        this._counts[MATCHTYPE.HEURISTIC] + this._counts[MATCHTYPE.GENERAL]

in both cases, I'd suggest to assign this._counts[MATCHTYPE.HEURISTIC] + this._counts[MATCHTYPE.GENERAL] to a local let count, to make the conditions more readable.

Comment 8

[Marco Bonardo [:mak]]

please update the patch so we can land it ;)

Comment 9

[Simon Lindholm]

Attachment: Use more precise criteria for stopping auto-complete searches

Right!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c721cddf0dbc
Use more precise criteria for stopping auto-complete searches. r=mak

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for failing browser-chrome's browser/base/content/test/performance/browser_urlbar_search_reflows.js, at least on OS X and Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8326390c9c7a80bcb5b885d0c1773e38c0c77e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c721cddf0dbc9294f472003dc031a451615d21ca&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126776233&repo=mozilla-inbound

09:13:59     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_urlbar_search_reflows.js | Unused expected reflow: [
09:13:59     INFO - 	"_handleOverflow@chrome://global/content/bindings/autocomplete.xml",
09:13:59     INFO - 	"handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml",
09:13:59     INFO - 	"_reuseAcItem@chrome://global/content/bindings/autocomplete.xml",
09:13:59     INFO - 	"_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml",
09:13:59     INFO - 	"_invalidate@chrome://global/content/bindings/autocomplete.xml",
09:13:59     INFO - 	"invalidate@chrome://global/content/bindings/autocomplete.xml"
09:13:59     INFO - ]

Please fix the issue and submit an updated patch. Thank you.

Comment 12

[Simon Lindholm]

Interesting, I didn't imagine this sort of thing would vary across platforms... I can reproduce it locally on 64-bit Linux, but Treeherder does not mention failures there (nor did it in the try run I did a few days ago).

Hypothesis: cancelling an SQL query is racy. Let me try to change the test to use fewer than nine awesomebar entries and see if that helps.

Comment 13

[Marco Bonardo [:mak]]

"Unused expected reflow" seems to indicate you actually caused that reflow to go away, that it's a positive thing, so the test just needs to be updated  and that reflow should be removed.

Comment 14

[Marco Bonardo [:mak]]

Though, it's interesting that this seems to remove the reflow that is happening most often?!
That'd be really surprising!!!

Comment 15

[Marco Bonardo [:mak]]

I commented in bug 1356532 about this, that bug is about to remove these reflows from the UI, so there may be an information exchange here to try to understand if this is really happening or it's a test artifact.

Comment 16

[Simon Lindholm]

(In reply to Simon Lindholm from comment #12)
> Interesting, I didn't imagine this sort of thing would vary across
> platforms... I can reproduce it locally on 64-bit Linux, but Treeherder does
> not mention failures there (nor did it in the try run I did a few days ago).
> 
> Hypothesis: cancelling an SQL query is racy. Let me try to change the test
> to use fewer than nine awesomebar entries and see if that helps.

... or maybe the apparent platform dependence is just because the test is disabled on Linux and opt OSX... (bug 1385932 and bug 1384582) I'll do another try run with just the reflow counter decreased.

Still the hypothesis seems pretty plausible, at least if/when bug 1393440 lands. Another possible fix for it might be to make sure that _onResultRow doesn't send any events or notifications or cause reflows when the match it adds is out of bounds.

(In reply to Marco Bonardo [::mak] from comment #14)
> Though, it's interesting that this seems to remove the reflow that is
> happening most often?!
> That'd be really surprising!!!

Well, it doesn't remove it, it just reduces it from 390 to 330. Not entirely sure why, but maybe _addMatch causes some event to be dispatched, which is caught by the dirty-all-frames-on-all-events event listener in head.js?

Comment 17

[Marco Bonardo [:mak]]

ah, if it reduces the value it could make more sense, we could show a result and then push it away when a new one arrives.
I thought it was reducing it to zero! :)

So yes, I suspect just changing the test to 330 will do.

Comment 18

[Simon Lindholm]

Try run for the test adjustment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5f6429026f26882c16a55839bbd44e02f4479c

And never mind the comment about cancel being racy, it clearly isn't. There's still quite a few things about these reflow counters that I don't understand, but nothing that needs to block landing...

Comment 19

[Marco Bonardo [:mak]]

It looks good on Try, I'd say we can land the updated version. I'm curious to check results of this on a couple probes I'm looking at.

Comment 20

[Simon Lindholm]

Attachment: Use more precise criteria for stopping auto-complete searches

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/299138aaaecb
Use more precise criteria for stopping auto-complete searches. r=mak

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing browser_urlbar_keyed_search_reflows.js failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d62e39d7aea59baf70e3fe1496707d5aa3b895c

https://treeherder.mozilla.org/logviewer.html#?job_id=127183936&repo=mozilla-inbound

Comment 23

[Marco Bonardo [:mak]]

These reflows tests thing is getting out of control :(
Maybe the base tree of the Try push was not up-to-date and missing this test that has been added on 23.

Comment 24

[Simon Lindholm]

Indeed... At least things might settle down a little after 57. I'm also a bit confused why the first mozilla-inbound push didn't catch that, but maybe it only shows the first error or something? Or I'm just missing it in the view.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=571d719991a19b59d52f98f5c1af8b2617141076

Comment 25

[Simon Lindholm]

Attachment: Use more precise criteria for stopping auto-complete searches

Third time's the charm?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/873139ecdbd2
Use more precise criteria for stopping auto-complete searches. r=mak

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/873139ecdbd2