Last Comment Bug 680402 - Hang on certain site
: Hang on certain site
Status: RESOLVED FIXED
: hang, perf, regression, testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: mozilla9
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks: 553981
  Show dependency treegraph
 
Reported: 2011-08-19 04:36 PDT by Alice0775 White
Modified: 2011-08-30 04:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WinDbg log (15.69 KB, application/x-zip-compressed)
2011-08-19 04:36 PDT, Alice0775 White
no flags Details
hang testcase.html (833 bytes, text/html)
2011-08-19 04:49 PDT, Alice0775 White
no flags Details
not hang testcase.html (807 bytes, text/html)
2011-08-19 04:49 PDT, Alice0775 White
no flags Details
patch, v1 - avoid calling SanitizeGlyphRuns repeatedly during textrun construction with multiple scripts (1.80 KB, patch)
2011-08-19 14:39 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review

Description Alice0775 White 2011-08-19 04:36:36 PDT
Created attachment 554368 [details]
WinDbg log

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f69a10f23bf3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110818 Firefox/9.0a1 ID:20110818030747

Browser hang




Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile
2. Open Page ( http://www.openspc2.org/JavaScript/benchmark/addText2/index.html )
3. 


Actual Results:
  Browser hang

Expected Results:
  Display page in about 3sec on 2.5GHz C2Q

Regression window(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/b2aeaf724c8d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208215553
Hang:
http://hg.mozilla.org/mozilla-central/rev/0567fd4cf088
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208230203
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2aeaf724c8d&tochange=0567fd4cf088

In local built(m-c repository)
built from 4861b72cc66d : hang
built from 0bd4cd152565 : fast

Triggered by:
4861b72cc66d	Jonathan Kew — bug 553981 - handle Hangul Jamo sequences and other special cases when marking clusters. r=karlt a=roc
Comment 1 Alice0775 White 2011-08-19 04:49:01 PDT
Created attachment 554372 [details]
hang testcase.html
Comment 2 Alice0775 White 2011-08-19 04:49:28 PDT
Created attachment 554373 [details]
not hang testcase.html
Comment 3 Jonathan Kew (:jfkthame) 2011-08-19 14:39:26 PDT
Created attachment 554556 [details] [diff] [review]
patch, v1 - avoid calling SanitizeGlyphRuns repeatedly during textrun construction with multiple scripts

The pathological behavior here arises when we try to construct a single textrun that has many separate script runs within it. The code ends up calling SanitizeGlyphRuns each time it appends a new glyph run, and that iterates over all the existing runs, which means we get behavior that is of the order of O(n^2) where n is the number of distinct glyph runs in the complete textrun, or something horrible like that. The testcase here is a worst-case scenario because it has mixed scripts repeated over and over within a single string for which we construct a giant textrun.

This patch should solve the problem by just calling SanitizeGlyphRuns once at the end of textrun construction, instead of repeating it for each added piece.
Comment 4 Jonathan Kew (:jfkthame) 2011-08-20 01:19:27 PDT
Tryserver builds with this patch available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-df8cd5194cfd/.
Comment 5 Alice0775 White 2011-08-22 22:52:42 PDT
The tryserver built is improved performance remarkably. (I expect early check-in!)
Comment 6 Jonathan Kew (:jfkthame) 2011-08-27 11:08:48 PDT
jdaggett, review ping? This is a simple fix for a bad regression....
Comment 7 John Daggett (:jtd) 2011-08-28 20:53:11 PDT
Comment on attachment 554556 [details] [diff] [review]
patch, v1 - avoid calling SanitizeGlyphRuns repeatedly during textrun construction with multiple scripts

Looks fine, but I think you should add a comment noting the details in comment 3 so that we remember what to avoid when restructuring code in the future.
Comment 8 Jonathan Kew (:jfkthame) 2011-08-29 02:31:08 PDT
Pushed to inbound, with extra comments:
http://hg.mozilla.org/integration/mozilla-inbound/rev/153a1de6cd67
Comment 9 Ed Morley [:emorley] 2011-08-30 04:34:12 PDT
http://hg.mozilla.org/mozilla-central/rev/153a1de6cd67

Note You need to log in before you can comment on or make changes to this bug.