421353 - Moving the mouse over text hyperlinks which become underlined spikes cpu usage

Status: VERIFIED FIXED

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

Description

[David Corley]

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)

Comment 1

[Mariana]

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 2

[Mariana]

Attachment: Mozilla_CPU

Comment 3

[Ria Klaassen (not reading all bugmail)]

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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

[Martijn Wargers (dead)]

Attachment: testcase

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.

Comment 6

[David Corley]

Testcase time for me:
FX2 = ~15000ms
FX3 = ~30000ms


That's pretty significant I would say?

Comment 7

[Ria Klaassen (not reading all bugmail)]

18246ms (repeated: 16653ms) = latest branch
13524ms (repeated: 12713ms) = pre bug 392785
28475ms (repeated: 28497ms) = post bug 392785
27648ms (repeated: 27558ms) = current trunk

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

o.k. but unfortunately, I cannot work on this after bug 417014...

Comment 17

[Steve Pick]

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.

Comment 18

[Martijn Wargers (dead)]

Can't the patch for bug 392785 be backed out? That doesn't seem like a major problem to me.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's a tough call. Incorrect repainting is bad, and I don't think this bug is all that bad.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v0.1

first shot.

I need more work and I need to test.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v1.0

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.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v1.0.1

oops, sorry. this is correct.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we're going to do this, why don't we just change the font metrics to include the text-decoration bottom?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

That's an option. But we still need the textframe part. Because textframe sometimes doesn't have the first font's metrics.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Er, that might be better. Because I change in gfx, it will help for XUL layout (when non-system fonts are used).

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.0

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.)

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v3.0

This patch also improve the selecting performance.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv3.0.1

removing a debug code, sorry for the spam.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // 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.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v3.1

Thank you roc. This patch also changes the reftest condition of z-index. Bug 388744 will be fixed by this patch.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v3.2

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.)

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311545 [details] [diff] [review]
Patch v3.2

a=beltzner

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in, I'll change the status if the all tests are passed.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

all green.

-> FIXED

Comment 41

[Jeremy Lea]

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

[Martijn Wargers (dead)]

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')

Comment 43

[David Corley]

I saw a drop from ~30,000ms to ~15,000ms.
Thanks Masayuki!

Comment 44

[Jeremy Lea]

Attachment: Fix incorrect use of units in previous patch

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.

Comment 45

[Reed Loden [:reed]]

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. :)

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 312794 [details] [diff] [review]
Fix incorrect use of units in previous patch

a1.9=beltzner

Comment 48

[Reed Loden [:reed]]

Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.176; previous revision: 3.175
done

Comment 49

[Jeff Walden [:Waldo]]

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.)

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Jeff Walden [:Waldo]]

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.

Comment 52

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This seems to have caused an acid2 regression -- see bug 426616?

Comment 53

[Boris Zbarsky [:bzbarsky]]

Also caused memory leak bug 449168.