1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

We currently invoke notifyResults for each new match we find.

This is done mostly to return results to the user as far as possible, but it has a cost, for each match we rebuild the whole ac results list, and it's quite expensive.

We can still retain responsiveness while adding some chunking, for example we could still return the heuristic result immediately, then chunk the next matches every n milliseconds.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

Review commit: https://reviewboard.mozilla.org/r/196644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196644/

Comment 2

[Drew Willcoxon :adw]

I understand the idea, but I think there's one problem with this implementation, at least potentially.  After the heuristic result, it's possible to starve the panel while the search is ongoing, for as long as NOTIFYRESULT_DELAY_MS * (maxResults - 1) ms.  I'm not sure whether that's acceptable.  I think not, but on the other hand, NOTIFYRESULT_DELAY_MS * (maxResults - 1) is only 16 * 9 = 144 ms.  Have you considered bounding the time between notifications in the worst case to some smaller ms?

Also, taking a step back:

(In reply to Marco Bonardo [::mak] from comment #0)
> but it has a cost, for each match we rebuild the whole ac results list,
> and it's quite expensive.

Can we just not do that?  Fixing that problem seems like the better solution to me.  But actually, could you point out where we rebuild the whole results list?  autocomplete-rich-result-popup appends new results as they come in, doesn't it?  Are you referring to some lower-level autocomplete component?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #2)
> I understand the idea, but I think there's one problem with this
> implementation, at least potentially.  After the heuristic result, it's
> possible to starve the panel while the search is ongoing, for as long as
> NOTIFYRESULT_DELAY_MS * (maxResults - 1) ms.

Do you mean in the case where the 9 matches are exactly spaced by 15ms?

> Have you considered bounding the
> time between notifications in the worst case to some smaller ms?

no, but it's a good idea!

> (In reply to Marco Bonardo [::mak] from comment #0)
> > but it has a cost, for each match we rebuild the whole ac results list,
> > and it's quite expensive.
> 
> Can we just not do that?  Fixing that problem seems like the better solution
> to me.

I thought about that, and I think we should ALSO do that (in bug 1356532 or follow-up).
Here I'm trying to avoid the calling overhead (XPConnect is expensive and here we cross it twice, JS -> CPP -> JS), plus the js code that brings us up to the rlb rebuild.

> But actually, could you point out where we rebuild the whole results
> list?  autocomplete-rich-result-popup appends new results as they come in,
> doesn't it? 

A result contains multiple matches, unifiedcomplete adds or replaces one of the matches and notifies that the result has been changed. The controller passes the result to autocomplete.xml, that walks all the RLB children. reusing one child may be cheap, but the whole process is not.

Comment 4

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #3)
> Do you mean in the case where the 9 matches are exactly spaced by 15ms?

Yes, exactly.  (Well, not necessarily *exactly* 15 ms.  More like 15.999... if timers were 100% precise, but in reality ~16 ms depending on each tick's slack.  But you get my point. :-))

> I thought about that, and I think we should ALSO do that (in bug 1356532 or
> follow-up).
> Here I'm trying to avoid the calling overhead (XPConnect is expensive and
> here we cross it twice, JS -> CPP -> JS), plus the js code that brings us up
> to the rlb rebuild.

Hmm, OK.  I'm not sure I buy that -- like, is adding a single result <= 16 ms?  (Or would it be once we fix the problem of rebuilding the whole results list for each result?)  I like the idea of adding results as soon as they're available, and I wouldn't want to give that up (absent some UX reason) without supporting data.  But I also don't want to block you here.  If it were me, I would try to fix that problem (of rebuilding the whole results list for each result) first, and then measure the overhead that you mention.  If it's bad, then I would come back to this bug.

I'll clear the review for now since it sounds like you're going to add support for bounding the max period of time between notifications?  (Which sounds tricky tbh.)  As I say, I don't want to block you here, so please r? again when you're ready.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #4)
> Hmm, OK.  I'm not sure I buy that -- like, is adding a single result <= 16
> ms?  (Or would it be once we fix the problem of rebuilding the whole results
> list for each result?)

16ms is likely the time we take just to display a frame (in the best case), so updating more often regardless doesn't make much sense.

> I like the idea of adding results as soon as they're
> available, and I wouldn't want to give that up (absent some UX reason)
> without supporting data.

I honestly think the difference will be negligible, because of the refresh rate, the user is unlikely to see results coming faster than 16ms. Sure, we could pile up 2 or 3 updates, and your idea to solve that sounds good.

But I also don't want to block you here.  If it
> were me, I would try to fix that problem (of rebuilding the whole results
> list for each result) first, and then measure the overhead that you mention.
> If it's bad, then I would come back to this bug.

I think this is a good patch, it's relatively low risk, doesn't prevent further optimizations UI side, allows to stop a search just with a stopSearch() call.
Consider all the involved code, then multiply the cost by 10 (one per match):
addMatch (JS)
 - XPConnect -
HandleSearchResult
ProcessResult (various XPConnect to check search status)
- XPConnect -
Invalidate
_appendCurrentResult (various XPConnect to the controller)
onResultsAdded
A frontend patch could only prevent part of the last 2 calls.

Btw yes, I'd like to clamp the timeout, likely to 16*3 ms.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8925501 [details]
Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196644/diff/1-2/

Comment 7

[Marco Bonardo [:mak]]

The patch clamps at 4 delays, if we are about to schedule a 4th delay, we just notify synchronously.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8925501 [details]
Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

hm looks like the latest change causes failures :(

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8925501 [details]
Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196644/diff/2-3/

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8925501 [details]
Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

https://reviewboard.mozilla.org/r/196644/#review203098

Thanks.

::: toolkit/components/places/UnifiedComplete.js:2407
(Diff revision 3)
> +    if (this._notifyTimer) {
> +      this._notifyTimer.cancel();
> +    }
> +    // In the worst case, we may get evenly spaced matches that would end up
> +    // delaying the UI by N_MATCHES * NOTIFYRESULT_DELAY_MS. Thus, we clamp the
> +    // numer of times we may delay matches.

Nit: "numer" typo

Comment 11

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #7)
> The patch clamps at 4 delays, if we are about to schedule a 4th delay, we
> just notify synchronously.

That's a good way to do it.  I was thinking of having another timer, but that's better.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8925501 [details]
Bug 1402286 - chunk notifyResults calls so that we don't run all the autocomplete js for each match.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196644/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14e4883dc598
chunk notifyResults calls so that we don't run all the autocomplete js for each match. r=adw

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/14e4883dc598