55.22 KB, text/html
61.42 KB, text/html
3.91 KB, patch
|Details | Diff | Splinter Review|
1.10 KB, patch
|Details | Diff | Splinter Review|
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060412 Firefox/3.0a1 For me, if I leave it for 1 minute 30 seconds, then the CPU usage comes back down from 100% and the source is displayed. However, I'm going to go ahead and confirm this because it's an excessive amount of time for a 55 KiB file (not overly big), just complex in nature).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
I don't see a hang in non-cairo builds.
Component: View Source → GFX: Thebes
Product: Firefox → Core
Version: unspecified → Trunk
On a (non-Cairo) trunk Mac build, viewing the source for the testcase took me 3 minutes and 15 seconds (195 seconds, during which the CPU is maxed). So this might not really be a Cairo-only thing. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060410 Firefox/3.0a1
Something very wrong is going on here in ComputeTotalWordDimensions / ComputeWordFragmentDimensions, although I don't understand this code well enough to say exactly where the problem is. ComputeTotalWordDimensions is calling ComputeWordFragmentDimensions repeatedly, for each of the bidi continuation frames. But ComputeWordFragmentDimensions doesn't actually measure the text in that frame. Instead, it measures what seems to be a block of 4096 characters from the text (containing the frame), and therefore it keeps returning the width of that block to ComputeTotalWordDimensions, which keeps accumulating this in addedDimensions, which therefore reaches ridiculously high values. Measuring the same (quite long) text over and over again is what's taking the long time (at least on my Mac). I'm not sure why the completely bogus returned dimensions don't have any effect on rendering.
Created attachment 218517 [details] testcase (without view-source) This is the <script> element from the original testcase, HTML-escaped, and embedded in <div style="white-space:pre;">. Rendering this testcase (not just it's source) takes a very long time.
Created attachment 218525 [details] [diff] [review] patch? This fixes it for me, by fixing what I described in the previous comment. Notice also the comment in: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.160&mark=2890-2891#2875 (which was later removed). I still don't feel I know this code well enough, so I can't vouch for the correctness of the patch.
Created attachment 218526 [details] [diff] [review] patch? The correct file, this time (hopefully).
I just noticed that the nsTextTransformer part of the patch is identical to what Simon suggested for bug 177442 (three and a half years ago).
Comment on attachment 218526 [details] [diff] [review] patch? looks good to me.
Uri, you're going to land this, right?
(In reply to comment #11) > Uri, you're going to land this, right? > Yes, but I want to get an OK from smontagu first (just to make sure there wasn't a good reason he didn't do this for bug 177442), and I also want to make sure I'm around to deal with possible regressions - meaning that I'll probably land it this weekend or early next week.
Sorry, I didn't know you were waiting for me. I held back in bug 177442 because I wanted to add similar logic to nsTextTransformer::ScanPreData_B, but by all means go ahead with this.
(In reply to comment #13) > Sorry, I didn't know you were waiting for me. I held back in bug 177442 because > I wanted to add similar logic to nsTextTransformer::ScanPreData_B, but by all > means go ahead with this. > I wasn't really waiting for you (and you couldn't have known, because I didn't tell you). I didn't want to land this this week anyway, as I'm often not at a computer where I can do debugging/checkins. I just hoped to catch you sometime on IRC or something and ask you. Anyway, thanks, and unless someone considers this urgent, I'll land it within a few days.
Checked in: Checking in layout/generic/nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.571; previous revision: 1.570 done Checking in layout/generic/nsTextTransformer.cpp; /cvsroot/mozilla/layout/generic/nsTextTransformer.cpp,v <-- nsTextTransformer.cpp new revision: 1.112; previous revision: 1.111 done Since the fix is not cairo-related, I changed the component to Bidi. Can someone please verify that this indeed fixed the performance problem on Windows? Also, it's very likely that cairo did actually regress the performance of bidi text-measuring on Windows, and while the current patch hopefully negates this regression in this case, we might want to look for other cases where the performance regression is apparent.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Component: GFX: Thebes → Layout: BiDi Hebrew & Arabic
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Verified. Source is displayed immediatly with "slim testcase" Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060422 Minefield/3.0a1 ID:2006042207 [cairo]
Either this or bug 334319 bumped up Tp by 3-5%.
(In reply to comment #17) > Either this or bug 334319 bumped up Tp by 3-5%. > Hm... almost certainly this one, I would say. Should I back it out, or is there some investigation that could be done first? How do I find out which pages specifically took the hit (or was this across the board)?
According to http://build-graphs.mozilla.org/graph/rawdata.cgi?tbox=argo.mozilla.org&testname=pageload&days=1 It seems the regression is entirely (or at least mostly) in lxr.mozilla.org which went from ~700 to ~2000.
The problem seems to be that for the non-bidi case, the frame content offsets are not yet set when we reach this code (at least for the first time), so |nextFrameStart + contentLen < nextFrameEnd| is always false (nextFrameEnd is always 0).
Created attachment 219446 [details] [diff] [review] patch for Tp regression This fixes the issue described in previous comment, which hopefully means it would fix the Tp regression.
For future reference: you can also find the per-page times in the full build log files (a bit easier to read): IDX PATH AVG MED MAX MIN TIMES ... 17 lxr.mozilla.org 1752 1710 1934 1694 1934 1710 1694 1716 1710 17 lxr.mozilla.org 2734 2690 2905 2681 2905 2690 2681 2706 2688
Regression patch checked in. I'll keep an eye on Tp to see how this goes. Checking in layout/generic/nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.572; previous revision: 1.571 done
The fix for the Tp issue worked, as far as I can tell. Thanks to jag for noticing the problem, and to roc for the quick review.
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: thebes → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.