Closed Bug 955957 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: cpeterson, Assigned: jfkthame)

References

()

Details

Attachments

(5 files, 2 obsolete files)

Attached file 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.
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.
(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.)
Component: Layout → Layout: Text
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.
(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.
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.
Assignee: nobody → jfkthame
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.
Attachment #8357110 - Flags: review?(roc)
Attachment #8356750 - Attachment is obsolete: true
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.
Attachment #8357203 - Flags: review?(roc)
Updated patch for the actual gfxSkipChars rewrite - just tidied up a bit, functionally identical to the earlier patch that was run through tryserver above.
Attachment #8357204 - Flags: review?(roc)
Attachment #8357110 - Attachment is obsolete: true
Attachment #8357110 - Flags: review?(roc)
And this updates the test to match the new API; still tests the same original<->skipped mappings.
Attachment #8357206 - Flags: review?(roc)
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
Attachment #8357203 - Flags: review?(roc) → review+
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
Attachment #8357204 - Flags: review?(roc) → review+
(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.
Depends on: 963878
Depends on: 970710
Depends on: 1012640
You need to log in before you can comment on or make changes to this bug.