Closed
Bug 907001
Opened 11 years ago
Closed 8 years ago
Autocomplete spends ages building text runs
Categories
(Firefox :: Address Bar, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 49
People
(Reporter: mattwoodrow, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [fxsearch])
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
adw
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
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
Reporter | ||
Comment 1•11 years ago
|
||
I mean, http://people.mozilla.com/~bgirard/cleopatra/#report=5b5216e2894ddd5641d561f09d95b436f84c532a
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?
Reporter | ||
Comment 3•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
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)
Reporter | ||
Comment 6•11 years ago
|
||
Yes, it appears to.
Comment 7•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
Marco, can you advise on how to best accomplish comment 7? Perhaps we can just tweak SQL_BASE in nsPlacesAutoComplete.js?
Flags: needinfo?(mak77)
Assignee | ||
Comment 10•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: [triage]
Updated•10 years ago
|
Whiteboard: [triage]
Updated•10 years ago
|
Whiteboard: p=0
Updated•10 years ago
|
Assignee | ||
Comment 13•10 years ago
|
||
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.
Updated•10 years ago
|
Points: --- → 5
Updated•10 years ago
|
Whiteboard: p=3
Updated•10 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: qe-verify+
Updated•10 years ago
|
QA Contact: alexandra.lucinet
Updated•10 years ago
|
QA Contact: alexandra.lucinet → andrei.vaida
Comment 14•10 years ago
|
||
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 → ---
Comment 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
Blocks: PlacesJank
Assignee | ||
Updated•9 years ago
|
Priority: -- → P1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mak77
Assignee | ||
Updated•8 years ago
|
Priority: P1 → P2
Whiteboard: [fxsearch]
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49455/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49455/
Attachment #8746553 -
Flags: review?(adw)
Comment 19•8 years ago
|
||
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+
Assignee | ||
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
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?
Assignee | ||
Comment 23•8 years ago
|
||
(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)
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70b3e6ac3839
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 26•8 years ago
|
||
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
Assignee | ||
Comment 27•8 years ago
|
||
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 28•8 years ago
|
||
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+
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment 29•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f66fd750140
Comment 30•8 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•