Closed Bug 907001 Opened 11 years ago Closed 8 years ago

Autocomplete spends ages building text runs

Categories

(Firefox :: Address Bar, defect, P2)

x86
macOS
defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox48 --- verified
firefox49 --- verified

People

(Reporter: mattwoodrow, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file)

At least with my current profile, I'm frequently getting multiple second hangs when typing into the awesome bar.

It appears to be spending the majority of the time within reflow, building text runs (~90% of the blocked time).

Profile: https://raw.github.com/bgirard/Gecko-Profiler-Addon/master/geckoprofiler.xpi
That's weird.

Maybe you could add logging to BuildTextRunsScanner::BuildTextRunForFrames to dump the text for the textruns we're creating, and add logging markers in PresShell::DoReflow to delimit the start and end of reflows, and dump that to a file so that next time you get this hang, we can try to figure out which textruns are being created and if there are too many?

Also, how often do these hangs occur? If you dismiss the awesomebar dropdown and then open it again immediately is it slow again?
Yep, it's slow every time, but depends on what characters I type. Starting to type 'inbound' is horrendously slow every time.

It appears that my history is full is ridiculously long reftest analyzer links, probably from copying the 'open reftest analyzer' link from  chrome.

I'd guess that these URL's are in the millions of characters long, and I'm getting a few of them displayed when trying to type inbound.

Given the massive range of random-ish characters in them, it's not surprising that they often turn up when typing other URL's too.

Limiting the text run to only the first few hundred characters that will actually be displayed would be nice, but I suspect I'm also losing pretty badly in the searching code.
Maybe we should just have awesomebar search ignore URLs longer than some threshold (say 1000 characters) (or possibly just truncate the part we search and display to 1000 characters?).

Gavin, who's the right person to make that call?
Flags: needinfo?(gavin.sharp)
If you modify "trimValue" at http://hg.mozilla.org/mozilla-central/annotate/14b1e8c2957e/browser/base/content/urlbarBindings.xml#l150 to truncate the string at 1000 characters (i.e. add a "aURL = aURL.substr(0, 1000);"), does the slowness go away?
Flags: needinfo?(gavin.sharp)
Yes, it appears to.
OK. This is an edge case, but I guess we should have the autocomplete back-end refuse to return very long URIs as results.
Component: Layout: Text → Location Bar
Product: Core → Firefox
I think I just hit this bug. I managed to track down a reftest analyzer page and close it and things sped right up again, but it was really bad while it was happening and a real user might take a long time to stumble over the fix.
Marco, can you advise on how to best accomplish comment 7? Perhaps we can just tweak SQL_BASE in nsPlacesAutoComplete.js?
Flags: needinfo?(mak77)
it should be easy to tweak either the query or the result fetcher to limit size of uris, though I wonder if we'll get many bug reports like "I visited page X but it doesn't appear in autocomplete". On the other side it may be very hard to crop uris without the risk to make them invalid.
Adding a simple check like LENGTH(h.url) < N to SQL_BASE would be easy, and shouldn't affect performances, since we are already doing a table scan here, but as I said, may generate surprising results: "I tagged page X, but it doesn't appear if I search tags", "I bookmarked page Y, but it doesn't appear if I search bookmarks".
The half way may be to add that check in ADDITIONAL_CONDITIONS only for _defaultQuery and _historyQuery, since results from these are harder to guess. We should probably still return any bookmark or tag or typed page.
Flags: needinfo?(mak77)
If we set the limit high enough, I suspect it won't be a big deal.

It seems like the only cases we really need to care about are history results and switch-to-tab results, it's probably not critical to address the case where someone bookmarks a very long URL.
Whiteboard: [triage]
Whiteboard: [triage]
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=3
I think we should try to do 2 things:
1. limit search into the first N chars of the url. The reasoning here is that it's unlikely the user could match on purpose something at the end of a very long url he can't even see or visually parse. On the other hand by doing this filtering we'd get faster and better matching results in most cases.
2. We could use finalCompleteValue to hold the whole url, while putting a trimmed url in value, this way we should not lose anything.

I can try to make an experimental patch soon.
Points: --- → 5
Whiteboard: p=3
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: qe-verify+
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
Sorry, I'll have to drop this from the current iteration. Should be able to pick it up next iteration again.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Iteration: 35.2 → ---
I won't get to this anytime soon. Alex, what does your current workload look like? Would you be interested in picking this up? Marco has a pretty solid idea of what needs to be done here and could surely mentor you. He outlined what to do in comment #13.
Flags: needinfo?(abardas)
(In reply to Tim Taubert [:ttaubert] from comment #15)
> I won't get to this anytime soon. Alex, what does your current workload look
> like? Would you be interested in picking this up? Marco has a pretty solid
> idea of what needs to be done here and could surely mentor you. He outlined
> what to do in comment #13.

I'm committed to some work right now, but I may be able to start working on it at the end of this week / next week.
Flags: needinfo?(abardas)
Blocks: 1071461
Blocks: PlacesJank
Priority: -- → P1
Keywords: perf
Blocks: 1214723
Assignee: nobody → mak77
Priority: P1 → P2
Whiteboard: [fxsearch]
Status: NEW → ASSIGNED
Comment on attachment 8746553 [details]
MozReview Request: Bug 907001 - Location bar is slow with long text runs.r=adw

https://reviewboard.mozilla.org/r/49455/#review46353
Attachment #8746553 - Flags: review?(adw) → review+
NI on myself, cause after some testing in Nightly I think it would be a good idea to uplift this, along with the new design.
Flags: needinfo?(mak77)
Marco, looks like we can back out https://bugzilla.mozilla.org/show_bug.cgi?id=804968 now that this landed. Backing it out would fix bug 901946. Do you agree?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Marco, looks like we can back out
> https://bugzilla.mozilla.org/show_bug.cgi?id=804968 now that this landed.
> Backing it out would fix bug 901946. Do you agree?

That bug is about calculating heights of the rows, but looks like getBoundingClientRect was very expensive due to their length instead?
If you can confirm that the problem doesn't exist anymore with this fixed, I'd all be for backing out that bug, since that may also help having rows of different height, that in the end is one of the requests for Activity Stream.
Flags: needinfo?(mak77) → needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/70b3e6ac3839
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Filed bug 1269333 in response to comment #23.
Flags: needinfo?(jaws)
This issue is verified fixed on latest Nightly 49.0a1 using: 
- Windows 10 x64
- Mac OS X 10.8.5
- Ubuntu 14.04 x86.

I also verified this issue on 
- latest Aurora 48.0a2
- 47.0b2 build1 (20160502152141)
- 46.0.1 build1 (20160502172042)
using the same OS's.
Status: RESOLVED → VERIFIED
Comment on attachment 8746553 [details]
MozReview Request: Bug 907001 - Location bar is slow with long text runs.r=adw

Approval Request Comment
[Feature/regressing bug #]: performance fix
[User impact if declined]: The awesomebar is slower than it could be. The main reason for this approval request is that we have a new awesomebar design in 48, and we'd like to couple that with this performance fix.
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: The patch mostly crops the strings we show in the UI, it doesn't have known downsides so far.
[String/UUID change made/needed]: none
Attachment #8746553 - Flags: approval-mozilla-aurora?
Comment on attachment 8746553 [details]
MozReview Request: Bug 907001 - Location bar is slow with long text runs.r=adw

Verified on nightly, perf fix for awesomebar, please uplift to aurora
Attachment #8746553 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I verified this issue on 48.0b4 build1 (20160627144420) using Mac OS X 10.11. The fix works as expected. I'm setting the corresponding flag.
Flags: qe-verify+
Depends on: 1414246
See Also: → 1540861
You need to log in before you can comment on or make changes to this bug.