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

RESOLVED FIXED in mozilla29

Status

()

Core
Layout: Text
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: jfkthame)

Tracking

Trunk
mozilla29
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Created attachment 8355134 [details]
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.
Created attachment 8355139 [details]
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.
(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
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8356750 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
(Assignee)

Comment 7

4 years ago
Created attachment 8357110 [details] [diff] [review]
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.
Attachment #8357110 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #8356750 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8357203 [details] [diff] [review]
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.
Attachment #8357203 - Flags: review?(roc)
(Assignee)

Comment 9

4 years ago
Created attachment 8357204 [details] [diff] [review]
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.
Attachment #8357204 - Flags: review?(roc)
(Assignee)

Updated

4 years ago
Attachment #8357110 - Attachment is obsolete: true
Attachment #8357110 - Flags: review?(roc)
(Assignee)

Comment 10

4 years ago
Created attachment 8357206 [details] [diff] [review]
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.
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+
Attachment #8357206 - Flags: review?(roc) → review+
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
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
Target Milestone: --- → mozilla29
had to backout this changes for build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32804376&tree=Mozilla-Inbound
(Assignee)

Comment 16

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/f9e848547525
https://hg.mozilla.org/mozilla-central/rev/58d7421ad39d
https://hg.mozilla.org/mozilla-central/rev/95f43cea056e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 963878

Updated

4 years ago
Depends on: 970710
Depends on: 1012640
You need to log in before you can comment on or make changes to this bug.