Closed
Bug 421353
Opened 17 years ago
Closed 17 years ago
Moving the mouse over text hyperlinks which become underlined spikes cpu usage
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: davidcorley, Assigned: masayuki)
References
()
Details
(Keywords: perf, regression, testcase)
Attachments
(4 files, 7 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
pavlov
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre Creative ZENcast v2.01.01 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre Creative ZENcast v2.01.01 If I move the mouse over any list of text hyperlinks on the BBC homepage (or any other page where the hyperlinks are in text format and become underlined on hover-over) the cpu usage spikes. This behaviour doesn't appear when the hyperlinks are not in text format Reproducible: Always Steps to Reproduce: 1.Go to http://news.bbc.co.uk/ 2.Move your mouse cursor rapidly over text links on the page 3.CPU usage will spike as long as you continue to move over text hyperlinks Actual Results: CPU usage shoots up to ~60/70% Expected Results: I would expect CPU usage to be close to 0% for such trivial actions I'm using the default theme. My CPU is a hyper-threading 3.8Ghz P4. I'm using NOD32 anti-virus 3.0.642, spyware blaster 4.0, spybot search & destroy 1.5.2, and ad-aware 2007 on the system. My GPU is an Nvidia 6800 Go with driver version 97.44 I'm running the latest release candidate of XP SP3 (v3300)
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre I noticed the same problem when I move my mouse cursor over the links on http://www.ebay.com/ My CPU usage shoots up to 66%
Comment 3•17 years ago
|
||
Confirmed with latest trunk on Windows XP. Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1203145320&maxdate=1203176399 with Bug 392785 as the most likely cause.
Assignee | ||
Comment 4•17 years ago
|
||
Because we recompute the underline painting area strictly. The old implementation didn't do so. Therefore, we could not paint underlines of similar style with some fonts. I think that this is INVALID (or WONTFIX).
Comment 5•17 years ago
|
||
With a 2008-02-15 build I get 10259ms as result.
With a 2008-02-15 build I get 12260ms as result.
With a 2008-03-07 build I get 11411ms as result (PGO really seems to have made things faster).
> I think that this is INVALID (or WONTFIX).
How can a performance regression be INVALID?
Isn't there some kind of optimization that can be done? I mean, it wasn't so that older builds didn't work with underlines.
Updated•17 years ago
|
Reporter | ||
Comment 6•17 years ago
|
||
Testcase time for me: FX2 = ~15000ms FX3 = ~30000ms That's pretty significant I would say?
Comment 7•17 years ago
|
||
18246ms (repeated: 16653ms) = latest branch 13524ms (repeated: 12713ms) = pre bug 392785 28475ms (repeated: 28497ms) = post bug 392785 27648ms (repeated: 27558ms) = current trunk
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #5) > Isn't there some kind of optimization that can be done? I mean, it wasn't so > that older builds didn't work with underlines. I have no idea. Because style system doesn't know whether the recomputing is needed.
Assignee | ||
Comment 9•17 years ago
|
||
There might be one way. All frams always contain the normal underline painting area in overflow rect. At this case, we don't need to reflow at changing the text-decoration value. However, it might create another performance regression.
That would be a complete disaster, it's not an option. One thing we could try is modifying nsStyleContext::CalcDifference to pass the style context as a parameter to nsStyleTextReset::CalcDifference. Then, if the text-decoration value is changing, nsStyleTextReset::CalcDifference could check the font to see if the font allows the underline to extend below the descent; if it doesn't, we don't need to return a REFLOW hint. Actually I guess it would need to check the old and new font. David, would a change like that violate some style-system architectural constraint? It doesn't seem so to me, but I'm not 100% sure.
Assignee | ||
Comment 11•17 years ago
|
||
roc: I'm not sure what you think. The underline position and the size depend on the font-family, lang and also bug 417014. So, it's too complex.
Yeah, that's true. :-( I hope we can get away without fixing this for Gecko 1.9. One possible fix would be to introduce a new style change hint that means "reflow if underline extends beyond frame descent" ... we could then ask the frame whether it really needs reflow in this situation. Another, probably better fix would be to change the way we do overflow areas and make it possible to change certain kinds of overflow areas, like this one, without doing reflow. We'd have to give up on extending scroll areas to scroll to see the underline, which might or might not be acceptable.
Assignee | ||
Comment 13•17 years ago
|
||
roc: How about we always contain the text-decoration area to the frame rect? That changes the line-height of some pages. But it's not a problem, I think.
I suppose that might work. Just be careful, there are places in textframe where we assume that the frame ascent + descent is equal to the height.
Assignee | ||
Comment 15•17 years ago
|
||
o.k. but unfortunately, I cannot work on this after bug 417014...
Assignee | ||
Updated•17 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•17 years ago
|
Component: General → Layout: Fonts and Text
OS: Windows XP → All
QA Contact: general → layout.fonts-and-text
Hardware: PC → All
Updated•17 years ago
|
Flags: blocking1.9?
Comment 17•17 years ago
|
||
I note this does not seem to occur for all websites. Wikipedia has serious issues with this bug causing 100% CPU usage on my fully-patched XP machine with 768mb RAM and a 2ghz CPU (actually makes my mouse cursor lag). eBuyer.com, however, does not have the issue on the same kind of links at the bottom of the homepage.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Comment 18•17 years ago
|
||
Can't the patch for bug 392785 be backed out? That doesn't seem like a major problem to me.
It's a tough call. Incorrect repainting is bad, and I don't think this bug is all that bad.
Assignee | ||
Comment 20•17 years ago
|
||
yeah, and I need to create a new patch for backing out. Because the related code were changed after bug 392785 (especially, the underline of the blacklisted fonts in bug 417014 is positioned to overflow area!). So the patch might be risky. And I'm trying to create a patch for this now. We have some risk on both way, so, I think I should try to fix this bug, not backing out.
Assignee | ||
Comment 21•17 years ago
|
||
first shot. I need more work and I need to test.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•17 years ago
|
||
The patch has two side effects. 1. textframe always has the first font's ascent (for overline) and descent (for underline). Therefore, bug 412251 will be fixed by the patch. 2. after bug 417014, the underline of major CJK fonts are painted on overflowed area. And the outline is painted the edge of overflowed area. But the patch reduces the overflow area, so, CJK text has enough line gap even if the text has underline. It looks nice for me.
Blocks: 412251
Assignee | ||
Comment 23•17 years ago
|
||
o.k. let's take this. 1. backing-out the CalcDifference changes and calling CombineTextDecorations codes from linelayout and blockframe. 2. textframe, inlineframe and linebox contain the text-decoration painting area in their bounds. 3. textframe still needs to combine the text-decoration area for the thick underline of IME. 4. if font-size:0;, ComputeNormalTextDecorationPaintableArea always returns zero for ascent and descent.
Attachment #309490 -
Attachment is obsolete: true
Attachment #309669 -
Flags: superreview?(roc)
Attachment #309669 -
Flags: review?(roc)
Assignee | ||
Comment 24•17 years ago
|
||
oops, sorry. this is correct.
Attachment #309669 -
Attachment is obsolete: true
Attachment #309672 -
Flags: superreview?(roc)
Attachment #309672 -
Flags: review?(roc)
Attachment #309669 -
Flags: superreview?(roc)
Attachment #309669 -
Flags: review?(roc)
If we're going to do this, why don't we just change the font metrics to include the text-decoration bottom?
Assignee | ||
Comment 26•17 years ago
|
||
That's an option. But we still need the textframe part. Because textframe sometimes doesn't have the first font's metrics.
Assignee | ||
Comment 27•17 years ago
|
||
Er, that might be better. Because I change in gfx, it will help for XUL layout (when non-system fonts are used).
Assignee | ||
Comment 28•17 years ago
|
||
roc: Oops, I forgot "bad" underline fonts. So, the underline might not be first font's underline. So, we *cannot* fix this bug in gfx level. The current patch approach is correct.
But we could store the adjusted ascent and descent in the gfxFontGroup and get them from there, right, just like we do with the underline offset? That would make this less scary for performance. And we could make fm->GetMaxAscent/GetHeight use those values so we wouldn't have to change many callers.
Assignee | ||
Comment 30•17 years ago
|
||
O.K. This fixes this bug on gfx level. However, I need to clear the metrics of font-size: 0;. Because I cannot clear layout/reftests/bugs/386065-1.html without that. It might create some new crash bugs (zero divid). And that can pass layout/reftests/z-index/z-index-1.html. (So, I need to change the reftest condition.)
Attachment #309672 -
Attachment is obsolete: true
Attachment #310354 -
Flags: superreview?(roc)
Attachment #310354 -
Flags: review?(roc)
Attachment #309672 -
Flags: superreview?(roc)
Attachment #309672 -
Flags: review?(roc)
Assignee | ||
Comment 31•17 years ago
|
||
This patch also improve the selecting performance.
Attachment #310354 -
Attachment is obsolete: true
Attachment #310439 -
Flags: superreview?(roc)
Attachment #310439 -
Flags: review?(roc)
Attachment #310354 -
Flags: superreview?(roc)
Attachment #310354 -
Flags: review?(roc)
Assignee | ||
Comment 32•17 years ago
|
||
removing a debug code, sorry for the spam.
Attachment #310439 -
Attachment is obsolete: true
Attachment #310441 -
Flags: superreview?(roc)
Attachment #310441 -
Flags: review?(roc)
Attachment #310439 -
Flags: superreview?(roc)
Attachment #310439 -
Flags: review?(roc)
+ // If the underline is overflowed, that should be in descent space. See bug421353. + if (aMetrics->underlineSize - aMetrics->underlineOffset > aMetrics->maxDescent) { + gfxFloat extra = aMetrics->underlineSize - aMetrics->underlineOffset - aMetrics->maxDescent; + aMetrics->maxDescent += extra; + aMetrics->maxHeight += extra; + } Is this actually needed? + PRBool hasDecorationArea = !!(mState & TEXT_SELECTION_UNDERLINE_OVERFLOWED); + PRBool hasSelectedText = aSelected && HasSelectionUnderline(); These be named "didHaveSelectionUnderline", "willHaveSelectionUnderline" to indicate that they represent before and after states. nsTextFrame::UnionTextDecorationOverflow will add the area for any selected content. Shouldn't nsTextFrame::UnionTextDecorationOverflow be calling HasSelectionUnderline? And HasSelectionUnderline should be called HasSelectionOverflowingDecorations, I think ... I think we should move the nsILookAndFeel > 1.0 check into it. I don't think we should check for editable state here, because that means editable state can affect reflow which sounds bad. We should just ensure that any text which is not selected-with-underline does not have the extra decoration area added to it. aMetrics.ascent = NSToCoordCeil(textMetrics.mAscent); Shouldn't you remove this line? + nscoord minAscent, minDescent; + nsCOMPtr<nsIFontMetrics> fm; + nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fm)); There should be a way to avoid calling this, again during reflow. You may need to refactor GetFontGroupForFrame perhaps by allowing it to return the font metrics as an out parameter.
Assignee | ||
Comment 34•17 years ago
|
||
Thank you roc. This patch also changes the reftest condition of z-index. Bug 388744 will be fixed by this patch.
Attachment #310441 -
Attachment is obsolete: true
Attachment #311365 -
Flags: superreview?(roc)
Attachment #311365 -
Flags: review?(roc)
Attachment #310441 -
Flags: superreview?(roc)
Attachment #310441 -
Flags: review?(roc)
Comment on attachment 311365 [details] [diff] [review] Patch v3.1 + if (aOutFontMetrics) + *aOutFontMetrics = metricsRaw; You should addref this before returning it and use nsCOMPtr/getter_AddRefs in the caller. Looks good with that.
Attachment #311365 -
Flags: superreview?(roc)
Attachment #311365 -
Flags: superreview+
Attachment #311365 -
Flags: review?(roc)
Attachment #311365 -
Flags: review+
Assignee | ||
Comment 36•17 years ago
|
||
Thank you roc. And I also need Stuart's review for gfx part. Stuart: This changes the nsThebesFontMetrics returns the underline/overline drawable area included values at MaxAscent/MaxDescent/MaxHeight/Height. That helps layout code. And I fill the gfxFont::Metrics to zero if the font size is zero. This thing fixes some bugs of zero font size layout. I worry about the new zero dividing crash, however, I don't find such regressions now. (Without this thing, this patch cannot pass the reftests.)
Attachment #311365 -
Attachment is obsolete: true
Attachment #311545 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #311545 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 37•17 years ago
|
||
Comment on attachment 311545 [details] [diff] [review] Patch v3.2 This improve the performance at changing the decoration lines (e.g., underline) on every elements. And this also improve the text selection performance. And also this fixes bug 412251 and bug 424249. The risk is low.
Attachment #311545 -
Flags: approval1.9?
Comment 38•17 years ago
|
||
Comment on attachment 311545 [details] [diff] [review] Patch v3.2 a=beltzner
Attachment #311545 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 39•17 years ago
|
||
checked-in, I'll change the status if the all tests are passed.
Assignee | ||
Comment 40•17 years ago
|
||
all green. -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 41•17 years ago
|
||
On lines 5591 and 5816 of the latest revision, there is the same construct: nsRect boundingBox = ConvertGfxRectOutward(textMetrics.mBoundingBox + gfxPoint(0, mAscent)); these should be nsRect boundingBox = ConvertGfxRectOutward(textMetrics.mBoundingBox) + nsPoint(0, mAscent); since mAscent is in app units, not device pixels. Sorry, I can't make a patch at the moment...
Comment 42•17 years ago
|
||
With a 2008-03-27 build, I get: 11139ms With a 2008-03-30 build, I get: 8807ms Masayuki, thanks for fixing this! (and sorry if I was a bit too 'pushy')
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 43•17 years ago
|
||
I saw a drop from ~30,000ms to ~15,000ms. Thanks Masayuki!
Comment 44•17 years ago
|
||
This fixes up some issues with the previous patch. It added an identical static helper function to an existing function, and then used it incorrectly. See my previous comment.
Attachment #312794 -
Flags: superreview?
Attachment #312794 -
Flags: review?
Comment 45•17 years ago
|
||
Comment on attachment 312794 [details] [diff] [review] Fix incorrect use of units in previous patch Next time, please file a new bug for these types of fixes. :)
Attachment #312794 -
Flags: superreview?(roc)
Attachment #312794 -
Flags: superreview?
Attachment #312794 -
Flags: review?(roc)
Attachment #312794 -
Flags: review?
Comment on attachment 312794 [details] [diff] [review] Fix incorrect use of units in previous patch mBoundingBox is actually in appunits, so there isn't really a bug here, but this patch is still good, eliminating the duplicated code and making this code consistent with ComputeTightBounds.
Attachment #312794 -
Flags: superreview?(roc)
Attachment #312794 -
Flags: superreview+
Attachment #312794 -
Flags: review?(roc)
Attachment #312794 -
Flags: review+
Updated•17 years ago
|
Attachment #312794 -
Flags: approval1.9?
Comment 47•17 years ago
|
||
Comment on attachment 312794 [details] [diff] [review] Fix incorrect use of units in previous patch a1.9=beltzner
Attachment #312794 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 48•17 years ago
|
||
Checking in layout/generic/nsTextFrameThebes.cpp; /cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v <-- nsTextFrameThebes.cpp new revision: 3.176; previous revision: 3.175 done
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
Comment 49•17 years ago
|
||
The number of gfxTextRuns leaked during Mochitest runs on the pgo box changed "when" (a several-hour range, but this was the only substantive change that might actually have affected this) this patch landed: random-seventy-two:~/moz/inflight/pgo-leaks jwalden$ diff -U 2 smaller.txt next.txt --- smaller.txt 2008-04-04 02:31:11.000000000 -0400 +++ next.txt 2008-04-04 12:16:59.000000000 -0400 @@ -1,3 +1,3 @@ -WARNING leaked 293433 bytes during test execution +WARNING leaked 299413 bytes during test execution WARNING leaked 103 instances of AtomImpl with size 16 bytes each (1648 bytes total) WARNING leaked 1 instance of BackstagePass with size 20 bytes @@ -30,6 +30,6 @@ WARNING leaked 3 instances of gfxGlyphExtents with size 40 bytes each (120 bytes total) WARNING leaked 12 instances of gfxImageFrame with size 56 bytes each (672 bytes total) -WARNING leaked 70 instances of gfxTextRun with size 80 bytes each (5600 bytes total) -WARNING leaked 16 instances of gfxTextRunFactory with size 8 bytes each (128 bytes total) +WARNING leaked 139 instances of gfxTextRun with size 80 bytes each (11120 bytes total) +WARNING leaked 22 instances of gfxTextRunFactory with size 8 bytes each (176 bytes total) WARNING leaked 12 instances of imgContainer with size 84 bytes each (1008 bytes total) WARNING leaked 1 instance of imgLoader with size 12 bytes @@ -181,5 +181,5 @@ WARNING leaked 3 instances of nsStaticCaseInsensitiveNameTable with size 48 bytes each (144 bytes total) WARNING leaked 1 instance of nsStreamConverterService with size 12 bytes -WARNING leaked 2113 instances of nsStringBuffer with size 8 bytes each (16904 bytes total) +WARNING leaked 2124 instances of nsStringBuffer with size 8 bytes each (16992 bytes total) WARNING leaked 25 instances of nsStringBundle with size 32 bytes each (800 bytes total) WARNING leaked 1 instance of nsStringBundleService with size 108 bytes @@ -187,5 +187,5 @@ WARNING leaked 12 instances of nsSupportsCStringImpl with size 20 bytes each (240 bytes total) WARNING leaked 1 instance of nsSystemPrincipal with size 32 bytes -WARNING leaked 675 instances of nsTArray_base with size 4 bytes each (2700 bytes total) +WARNING leaked 756 instances of nsTArray_base with size 4 bytes each (3024 bytes total) WARNING leaked 23 instances of nsTextNode with size 32 bytes each (736 bytes total) WARNING leaked 12 instances of nsThebesImage with size 96 bytes each (1152 bytes total) Might the 2008-04-04 02:19 checkin have caused it? It's possible we just haven't hit a steady state here, but 70->139 seems bigger than I'd expect even if the number is non-deterministic (but still possible; the first run leaked 110 gfxTextRuns). (So far the leak counts we've gotten there are 296853, 293433, 299413, and it's not yet obvious what's checkin-related versus non-determinism-related.)
I don't see anything which could have affected the lifetimes of gfxTextRuns. These numbers are taken from the end of the mochitest run?
Comment 51•17 years ago
|
||
Yeah; going by the full history from the box, it looks like there's just a lot of fluctuation in the numbers for a few classes (gfxTextRun, gfxTextFactory, nsStringBuffer, and nsTArray_base), apparently more or less randomly. :-\ Every single one of the last six runs has leaked a different number of bytes due almost entirely to fluctuations in the leak counts for these classes. From the looks of things, we'll need to fix a root problem or two before we can get reliable leak numbers from that box, at least, on the main mochitest run.
Depends on: 426616
This seems to have caused an acid2 regression -- see bug 426616?
You need to log in
before you can comment on or make changes to this bug.
Description
•