Closed
Bug 1408854
Opened 6 years ago
Closed 6 years ago
Very slow tab-switching when a tab with a large data URI is open
Categories
(Firefox :: Tabbed Browser, defect, P1)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | verified |
People
(Reporter: standard8, Assigned: jfkthame)
References
Details
(Keywords: perf, Whiteboard: [reserve-photon-performance])
Attachments
(1 file, 1 obsolete file)
1.79 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
I've seen this a couple of times now, but only just figured out what it was. STR: 1) On an existing profile, go to about:telemetry 2) At the bottom-left, click "Raw JSON" -> A new tab opens with the data URI for the raw JSON. 3) Try switching tabs Actual Results -> 2-3 second tab switch time. Expected Results -> Tab switch speed not affected. 4) Close the data URI tab -> Tab switch performance reverts to normal Here's a couple of profiles: https://perfht.ml/2yrl1w9 https://perfht.ml/2yqOhCY
Reporter | ||
Comment 1•6 years ago
|
||
Forgot to mention, this is using the latest nightly on Mac.
Comment 2•6 years ago
|
||
It's really bad that this affects all tab switches, not just switching to/from the one tab with the long URL. I wonder if it's related to bug 1387296... but even with that bug fixed we would still be slow to switch to the tab with the long URL. I think we should limit the length of the URL we show in the tab title in this code path: http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/base/content/tabbrowser.xml#1624 (In reply to Mark Banner (:standard8) from comment #0) > I've seen this a couple of times now, but only just figured out what it was. Thanks for figuring it out! :-) > Here's a couple of profiles: > > https://perfht.ml/2yrl1w9 > https://perfht.ml/2yqOhCY Most of the time is spent painting (in nsDisplayText::Paint). The rest is spent reflowing (in BuildTextRunsScanner::BuildTextRunForFrames). In addition to the front-end fix of limiting the tab title length, I wonder if there are platform optimizations that should be considered, to avoid spending so much time attempting to paint a single line of text that's obviously much longer than the screen.
Whiteboard: [photon-performance] [triage]
Comment 3•6 years ago
|
||
Yeah, this seems bad. Hey jfkthame, I know you dabble in text... do you know why we'd compute text rendering for the entirety of a string that's clearly going to be mostly offscreen?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #3) > Yeah, this seems bad. Hey jfkthame, I know you dabble in text... do you know > why we'd compute text rendering for the entirety of a string that's clearly > going to be mostly offscreen? I guess the short answer is that it would be really hard to -reliably- process only part of the string, without risking incorrect display in certain cases. How do we know it's going to be mostly offscreen until we've shaped and measured it? Just because it's a million characters doesn't guarantee anything; they could all be zero-width diacritics. :) And because of bidi reordering, even if we could safely assume that, say, no more than a couple of hundred characters are needed to fill the width that will be visible, we don't know (until we've done bidi resolution and reordering) which couple of hundred will end up visually at the side we're going to show. One thing that would probably help would be if the URL bar used an HTML frame rather than a XUL textbox, simply because that is more performant: it would still have to process all its text once, at least, but it then caches its textrun for future measurement and drawing operations (until style changes in some way that invalidates it, but that shouldn't be an issue for the URL bar). A XUL textbox doesn't do this, IIRC; it just does the text layout (bidi processing, glyph shaping) on-demand during each measurement or drawing operation using a transient textrun. So in practice it'll re-do the same work multiple times. (See also bug 837765.)
Flags: needinfo?(jfkthame)
Comment 5•6 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #4) > One thing that would probably help would be if the URL bar used an HTML > frame rather than a XUL textbox, simply because that is more performant: it > would still have to process all its text once, at least, but it then caches > its textrun for future measurement and drawing operations (until style > changes in some way that invalidates it, but that shouldn't be an issue for > the URL bar). A XUL textbox doesn't do this, IIRC; it just does the text > layout (bidi processing, glyph shaping) on-demand during each measurement or > drawing operation using a transient textrun. So in practice it'll re-do the > same work multiple times. <xul:textbox> uses <html:input> internally: http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/toolkit/content/widgets/textbox.xml#30
Comment 6•6 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #4) > One thing that would probably help would be if the URL bar used an HTML > frame rather than a XUL textbox I think what's being slow here is painting the tab title when redrawing the tab bar, not the urlbar.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 7•6 years ago
|
||
Ah, OK.... well, AFAICS that's an <xul:label>, which I think means comment 4 still applies -- just read "tab title" where I wrote "URL bar".
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 8•6 years ago
|
||
One somewhat ad hoc thing we could do to mitigate the specific case here would be to limit the tab-title length when it's a data: URI with base64 encoding, because in that case we can be confident we don't need to do bidi reordering or other complex text layout in order to find out exactly what gets displayed at the (visual) beginning; just truncating with an ellipsis after a few hundred characters should be fine. Doing that for arbitrary titles or IDNs is much less straightforward, but they're also less likely to be ridiculously long.
Assignee | ||
Comment 9•6 years ago
|
||
So if it's the tab title primarily causing the issue, something like this might help. (Not for every possible long title, of course, but for this specific type of example.)
Attachment #8919683 -
Flags: feedback?(florian)
Updated•6 years ago
|
Priority: -- → P3
Comment 10•6 years ago
|
||
Comment on attachment 8919683 [details] [diff] [review] PoC - Limit length of base64-encoded data: URI shown in tab title Review of attachment 8919683 [details] [diff] [review]: ----------------------------------------------------------------- This helps a lot, tab switching becomes instant for all tabs but the one with the giant data url. When switching to that tab, it takes about half a second on my fast macbook pro. Here is a profile of it: https://perfht.ml/2gSskn4 Most of the time is spent in nsTextFrame::ReflowText, I believe this is for the url bar. ::: browser/base/content/tabbrowser.xml @@ +1631,5 @@ > title = textToSubURI.unEscapeNonAsciiURI(characterSet, title); > + // If it's a long data: URI that uses base64 encoding, truncate to > + // a reasonable length rather than trying to display the entire thing. > + if (title.length > 500) { > + if (title.match(/^data:[^,]+;base64,/)) { nit: put these 2 tests on a single line with &&
Attachment #8919683 -
Flags: feedback?(florian) → feedback+
Updated•6 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [photon-performance] [triage] → [reserve-photon-performance]
Assignee | ||
Comment 11•6 years ago
|
||
OK, cleaned up the test; I also added a bit to the comment, as a reminder for anyone who stumbles across this in future.
Attachment #8920572 -
Flags: review?(florian)
Assignee | ||
Updated•6 years ago
|
Attachment #8919683 -
Attachment is obsolete: true
Comment 12•6 years ago
|
||
Comment on attachment 8920572 [details] [diff] [review] Limit length of base64-encoded data: URI shown in tab title Review of attachment 8920572 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8920572 -
Flags: review?(florian) → review+
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #10) > This helps a lot, tab switching becomes instant for all tabs but the one > with the giant data url. When switching to that tab, it takes about half a > second on my fast macbook pro. Here is a profile of it: > https://perfht.ml/2gSskn4 > Most of the time is spent in nsTextFrame::ReflowText, I believe this is for > the url bar. That seems likely, yes. I notice that of the ReflowText time, 70% is spent inside Core Text, shaping the humongous line of text for the URL bar. It would be interesting to check whether we're re-shaping the same text multiple times for any reason here; if so, that would be an inefficiency we should try to kill off. It may take some custom instrumentation to determine this, though. Anyhow, I'd suggest that if we want to investigate this any further we should spin it off to a followup.
Comment 14•6 years ago
|
||
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3653824994d5 Limit length of base64-encoded data: URI shown in tab title. r=florian
![]() |
||
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3653824994d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 16•6 years ago
|
||
Tested on latest Nightly 58.0a1 Build ID 20171102222620 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0 - Now, the switch time has significantly decreased (under 0.5 ms) when switching from a regular tab (regular URI) to the tab opened with the data URI for the raw JSON (Tab switch speed still slightly affected - as in not "instant" like with regular tabs) - When Switching between normal tabs (with the tab with the data URI for the raw JSON present/active) - the time is in normal performance parameters We noticed another performance issue related with this bug, which was logged separately. See bug 1414246. Thank you
Comment 17•6 years ago
|
||
(In reply to Deac Alin from comment #16) > Tested on latest Nightly 58.0a1 Build ID 20171102222620 Mozilla/5.0 > (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0 > - Now, the switch time has significantly decreased (under 0.5 ms) when > switching from a regular tab (regular URI) to the tab opened with the data > URI for the raw JSON (Tab switch speed still slightly affected - as in not > "instant" like with regular tabs) This is due to the long data url being displayed in the urlbar, not in tabs. I thought we had opened a follow-up bug to see if we can further improve this, bug I can't find it. > - When Switching between normal tabs (with the tab with the data URI for the > raw JSON present/active) - the time is in normal performance parameters This is what we fixed here, so I think this point is enough to mark the bug verified (unless you are intending to test on more platforms). > We noticed another performance issue related with this bug, which was logged > separately. See bug 1414246. Thanks for finding this! :-)
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•