Don't call searchFunction unnecessarily in MatchAutoCompleteFunction::OnFunctionCall

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We search for the each token in the record's tags, title, and URL, even if we could have stopped after we saw it in the tags or after we saw it in the title.

Fixing this probably wouldn't yield a substantial perf win, since most of the time we *don't* find what we're looking for, so we have to search everywhere.  Still, it seems like we shouldn't leave this on the table.
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #476413 - Flags: review?(sdwilsh)
(In reply to comment #0)
> We search for the each token in the record's tags, title, and URL, even if we
> could have stopped after we saw it in the tags or after we saw it in the title.
Wouldn't it be easier to just change this line:
if (HAS_BEHAVIOR(TITLE) && !matches)
to:
if (HAS_BEHAVIOR(TITLE) && matches)
and then update the comment accordingly?
(In reply to comment #2)
> Wouldn't it be easier to just change this line:
> if (HAS_BEHAVIOR(TITLE) && !matches)
> to:
> if (HAS_BEHAVIOR(TITLE) && matches)
> and then update the comment accordingly?

Well, then we wouldn't be able to short-circuit upon searchFunction(token, tags) returning true.
(In reply to comment #3)
> Well, then we wouldn't be able to short-circuit upon searchFunction(token,
> tags) returning true.
but the common case is that it'd be false, right?
(In reply to comment #4)
> but the common case is that it'd be false, right?

Yes, of course.  But by that logic, we could just WONTFIX and be done, right?

Maybe one way of looking at it is that the patch makes the intent of the code more clear, both to humans and to the compiler -- as a result, we get the added bonus of its running faster in some cases.
(In reply to comment #5)
> Maybe one way of looking at it is that the patch makes the intent of the code
> more clear, both to humans and to the compiler -- as a result, we get the added
> bonus of its running faster in some cases.
I spent a good ten minutes on the train verifying that both code paths did the same thing because I didn't think either one was terribly clear.  Adding comments to yours may help in that regard though.
Sure, I'll add a comment.
Posted patch Patch v2 (obsolete) — Splinter Review
Added comment.
Attachment #476413 - Attachment is obsolete: true
Attachment #480365 - Flags: review?(sdwilsh)
Attachment #476413 - Flags: review?(sdwilsh)
would make sense to search title before tags? not many pages have tags while all pages have titles, thus I'd expect that in most cases searchFunction(token, tags) is false and a useless call.
I thought about this, but it's probably not a big deal, since searchFunction will immediately return return false when it notices that the search string is empty.

If we really wanted to optimize it, I'd try writing three loops rather than branching within the loop.  But I've never seen this function in a profile (except as a caller of hot functions), so I was trying to resist the urge to optimize it (except by reducing the number of hot functions it calls, of course). The functions are only hot when they actually iterate over some history data.

Anyway, I'd be happy to make the change, if only so the order reflects our expectations.
(In reply to comment #10)
> Anyway, I'd be happy to make the change, if only so the order reflects our
> expectations.
Let's do that please.
Comment on attachment 480365 [details] [diff] [review]
Patch v2

r=sdwilsh with comment 11 addressed
Attachment #480365 - Flags: review?(sdwilsh) → review+
Posted patch Patch v3Splinter Review
Addressing review comments.
Attachment #480365 - Attachment is obsolete: true
Attachment #491702 - Flags: approval2.0?
Comment on attachment 491702 [details] [diff] [review]
Patch v3

Please don't request approval without indicating the risks/rewards/testing associated with the patch. It's not clear to me whether we can test this performance optimization, or whether there are tests which already adequately cover this condition.
Attachment #491702 - Flags: approval2.0?
sdwilsh, can you comment on the coverage of the testsuite with respect to this patch?

I don't expect this to have particularly large performance effects.  It's significant when you're searching for something which is common, such as the string "th" -- in this case, you get to skip many calls to searchFunction.  But if the search term is common, my understanding of the code is that you'll quickly find enough results to fill up the awesomebar, and then you'll stop searching.
(In reply to comment #15)
> sdwilsh, can you comment on the coverage of the testsuite with respect to this
> patch?
The location bar behavior is fully tested.  I'm not worried about any regressions here.
Comment on attachment 491702 [details] [diff] [review]
Patch v3

I'm re-requesting approval based on comment 16, even small performance gains in the locationbar can make a great difference, since it's one of the most used UI pieces in the browser. The autocomplete code has lots of tests, and it's easy to add further in case anything would break.
Attachment #491702 - Flags: approval2.0?
(In reply to comment #17)
> I'm re-requesting approval based on comment 16, even small performance gains in
> the locationbar can make a great difference, since it's one of the most used UI
> pieces in the browser. The autocomplete code has lots of tests, and it's easy
> to add further in case anything would break.
Note: I can't think of anything that isn't tested in this code path.  Luckily, the location bar algorithm was written well after we started focusing on tests landing with the code, so reviewers have made sure every new piece of functionality is tested.
I'm marking this with [check-in-after-branch] so I remember to come back to this after we branch 4.0, but I don't mean to negate mak's request for a2.0.
Whiteboard: [check-in-after-branch]
Assignee: nobody → justin.lebar+bug

Updated

9 years ago
No longer blocks: post2.0
Depends on: post2.0
Attachment #491702 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/85edf194b461
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [check-in-after-branch]
No longer depends on: post2.0
You need to log in before you can comment on or make changes to this bug.