955957 - Firefox hangs when displaying view-source of a 7 MB minified JS file

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load http://eaassets-a.akamaihd.net/bl-cdn/cdnprefix/c55546fd75648c6711c07ca4ddec5ae/public/generated/en_US/bundle_base_bottombundles_2770699820.js
2. Witness the minified JS code.
3. Load view-source:http://eaassets-a.akamaihd.net/bl-cdn/cdnprefix/c55546fd75648c6711c07ca4ddec5ae/public/generated/en_US/bundle_base_bottombundles_2770699820.js

RESULT:
Firefox hangs when trying for pretty print the minified JS.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: gzipped JS file

Here's a gzipped copy of the JS file, in case the remote copy disappears. I tried viewing it (unzipped) locally, triggering view-source, and I was able to reproduce the hang.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of sysprof profile

A sysprof profile shows we're spending basically all of our time in a reflow triggered by nsDocumentViewer::LoadComplete, and that time ends up mostly being spent in gfxTextRun::BreakAndMeasureText.

GDB confirms that this is the function that we're stuck in. (From interrupting the run arbitrarily and then typing "fin" until gdb stops giving me a prompt.)

We're stuck inside of this loop:

6273     uint32_t i;
6274     for (i = aStart; i < end; ++i) {
6275         if (i >= bufferStart + bufferLength) {
6276             // Fetch more spacing and hyphenation data
6277             bufferStart = i;
6278             bufferLength = std::min(aStart + aMaxLength, i + MEASUREMENT_BUFFER_SIZE) - i;
6279             if (haveSpacing) {
6280                 GetAdjustedSpacing(this, bufferStart, bufferStart + bufferLength, aProvider,
6281                                    spacingBuffer);
6282             }
6283             if (haveHyphenation) {
6284                 aProvider->GetHyphenationBreaks(bufferStart, bufferLength,
6285                                                 hyphenBuffer);
6286             }
6287         }
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#6273

...and "end" (the number of times to run the loop) is 6909897.

Comment 3

[Daniel Holbert [:dholbert]]

(er, I suppose 'i' starts at aStart, which it turns out is 1472114, so technically "end" isn't quite the number of times to run the loop, but you know what I mean.)

Comment 4

[Jonathan Kew [:jfkthame]]

So the problem is triggered by the last line of the minified JS, which is a single line that contains around 5.4 million characters of JS source.

From the profile, it looks like gfxSkipChars is becoming way too expensive when dealing with such a huge line. Maybe we can find a way to make it scale better? But we'll need to be careful not to regress performance on the common case of much shorter lines; IIRC, gfxSkipChars performance is pretty important in overall text perf.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (er, I suppose 'i' starts at aStart, which it turns out is 1472114, so
> technically "end" isn't quite the number of times to run the loop, but you
> know what I mean.)

The number of iterations of that loop wouldn't be a big issue in itself, if it weren't for the fact that the JS source text includes some literal <TAB> characters, which cause the text run to be created with the TEXT_ENABLE_SPACING flag, which in turn means that we call the PropertyProvider to get per-glyph spacing information. That in turn uses the gfxSkipChars to deal with the mapping between positions in the text run and character offsets in the original DOM text, and -that's- what is so expensive in this loop.

Replace all occurrences of <TAB> with <SPACE> in the file, for example, and the problem no longer occurs.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: reimplement gfxSkipChars and gfxSkipCharsIterator to perform better with huge text runs

Here's a patch that aims to reimplement gfxSkipChars in such a way that it avoids the performance black-hole here. I won't flag this for review until I see how it fares on tryserver, but in local testing it's looking good so far; View Source on the pathological JS file here renders in a second or so.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: reimplement gfxSkipChars and gfxSkipCharsIterator to perform better with huge text runs

Slightly updated patch, as unit tests found an edge-condition failure in the first version. With this change, the time to display View Source for the JS file here drops from over 9 *minutes* to about 1-2 *seconds* on my MBPro. (I don't see any significant change to 'normal' text performance, e.g. using John's textbench tool; the issue arises due to the ridiculously long non-wrapped line in the JS source.) Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=b77462ea3454.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 0 - add a simple unit test for gfxSkipChars functionality

Because the behavior of gfxSkipChars and gfxSkipCharsIterator can get a bit confusing (at least to me), I decided to write a simple testcase to help check that their behavior is still correct after the rewrite here.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - reimplement gfxSkipChars and gfxSkipCharsIterator to perform better with huge text runs

Updated patch for the actual gfxSkipChars rewrite - just tidied up a bit, functionally identical to the earlier patch that was run through tryserver above.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: pt 1.1 - update gfxSkipChars test for revised API (no gfxSkipCharsBuilder any longer)

And this updates the test to match the new API; still tests the same original<->skipped mappings.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8357203 [details] [diff] [review]
pt 0 - add a simple unit test for gfxSkipChars functionality

Review of attachment 8357203 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8357204 [details] [diff] [review]
pt 1 - reimplement gfxSkipChars and gfxSkipCharsIterator to perform better with huge text runs

Review of attachment 8357204 [details] [diff] [review]:
-----------------------------------------------------------------

This is very nice. Thanks!

Pathological strings could use a lot of memory, e.g. "a  b  c  d  e  f  " etc would use 4x as much memory for the gfxSkipChars as for the actual text (maybe more due to nsTArray slop). I agree not to worry about that for now.

We tried to use gfxSkipCharsIterator as an actual iterator, so stepping forward was basically constant time. Now you do a binary search at every step. It would be kinda nice to special-case the case where mCurrentRangeIndex remains valid. If you don't think that's worth doing I guess that's OK.

::: gfx/thebes/gfxSkipChars.cpp
@@ +113,2 @@
>          if (aRunLength) {
> +            uint32_t end = mSkipChars->mRanges.Length() > 0 ?

Prefer to call IsEmpty() than to compare Length() to zero

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 8357204 [details] [diff] [review]
> pt 1 - reimplement gfxSkipChars and gfxSkipCharsIterator to perform better
> with huge text runs
> 
> Review of attachment 8357204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is very nice. Thanks!
> 
> Pathological strings could use a lot of memory, e.g. "a  b  c  d  e  f  "
> etc would use 4x as much memory for the gfxSkipChars as for the actual text
> (maybe more due to nsTArray slop). I agree not to worry about that for now.

Right, this could indeed increase memory use in some cases (although there are other edge cases where it'd be somewhat reduced instead - consider a gfxSkipChars for a multi-megacharacter string, with a single skipped char near the end).

IMO, this is not fundamentally different from the more basic fact that you can always OOM the browser by stuffing sufficiently large amounts of text into the DOM; how fast you run out of memory will depend on the exact characteristics of the text involved, but that's true for other reasons as well - e.g. if you arrange that every character will have a DetailedGlyph record in the textrun, and that alternate characters come from two different fonts, hence generating separate GlyphRuns, we'll chew through memory significantly faster. But it's still essentially the same issue: "infinite amounts of content will eventually run out of space".

The hang with the view-source example here was a bit different, in that we were falling off a performance cliff long before we're in any danger of running out of memory. So that's why I thought it worth trying to fix.

> We tried to use gfxSkipCharsIterator as an actual iterator, so stepping
> forward was basically constant time. Now you do a binary search at every
> step. It would be kinda nice to special-case the case where
> mCurrentRangeIndex remains valid. If you don't think that's worth doing I
> guess that's OK.

I left this out for now, but we could do it as a followup if desired. If we find that gfxSkipCharsIterator::Set*Offset is significant in profiles of any particular testcase, we could experiment with this kind of optimization, but in practice I expect that the ranges array is often small enough that the binary search is scarcely more expensive than the tests to see whether the existing index is valid.

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab32fa7732f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b54755de2a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e285a5fbea6c

Comment 15

[Carsten Book [:Tomcat]]

had to backout this changes for build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32804376&tree=Mozilla-Inbound

Comment 16

[Jonathan Kew [:jfkthame]]

Re-landed, with the missing #include "mozilla/ArrayUtils.h" added to the testcase:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e848547525
https://hg.mozilla.org/integration/mozilla-inbound/rev/58d7421ad39d
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f43cea056e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9e848547525
https://hg.mozilla.org/mozilla-central/rev/58d7421ad39d
https://hg.mozilla.org/mozilla-central/rev/95f43cea056e