Mark Banner (:standard8) Reporter
	Description • 7 years ago

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

	Mark Banner (:standard8) Reporter
	Comment 1 • 7 years ago

Forgot to mention, this is using the latest nightly on Mac.

	Florian Quèze [:florian]
	Comment 2 • 7 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.

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 7 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?

	Jonathan Kew [:jfkthame] Assignee
	Comment 4 • 7 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.)

	Dão Gottwald [:dao]
	Comment 5 • 7 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

	Florian Quèze [:florian]
	Comment 6 • 7 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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 7 • 7 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".

	Jonathan Kew [:jfkthame] Assignee
	Comment 8 • 7 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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 9 • 7 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.)

	Florian Quèze [:florian]
	Comment 10 • 7 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 &&

	Jonathan Kew [:jfkthame] Assignee
	Comment 11 • 7 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.

	Florian Quèze [:florian]
	Comment 12 • 7 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!

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 7 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.

	Pulsebot
	Comment 14 • 7 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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3653824994d5

	Deac Alin
	Comment 16 • 7 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

	Florian Quèze [:florian]
	Comment 17 • 7 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! :-)