77939 - why does nsTextFrame::Reflow() need the text's color?

Status: RESOLVED DUPLICATE of bug 244651

(Core :: Layout, defect, P4)

Description

[Chris Waterson]

nsTextFrame's Reflow() method creates an nsTextStyle object, whose ctor looks up
the text's color. This seems unnecessary, and accounts for a noticable amount of
time laying out long pages (cf. bug 56854, about 1/20th of the time in reflow,
or ~1% overall).

Comment 1

[rbs]

Why should looking up the text's color be expensive in the first place? Isn't 
this readily available from the nsTextFrame' style context?

Comment 2

[Chris Waterson]

rbs, see bug 77941. I suspect what's happening is that we're asking for the 
default system text color, which requires us to go to the l&f service to resolve 
it.

Here's the relevant extract from bug 56854:

    nsTextFrame::Reflow() (sans text measurement) still accounts for
    30% of the inline reflow time, 208msec.

    . nsTextFrame's nsTextStyle ctor accounts for 45% of this
      (95msec), mostly because it has to get color (?) and font
      metrics information from the OS.

      - Getting the color information looks to fall through to the OS
        each time, which makes it more expensive that need be.

      - Furthermore, the implementation of nsXPLookAndFeel::GetColor()
        is atrocious given that we're calling this 88K times! This
        should a straight lookup, not a table crawl (which will *miss*
        most of the time).

      - But why do we even need color information at all here? [2]

      Probably the way to fix this is to make nsLookAndFeel (probably
      a singleton?) cache the color IDs in a table so it doesn't have
      to fall through to nsXPLookAndFeel *or* the OS. Getting color
      information accounts for 6% of the line reflow time (40msec, a
      bit under 1% of the overall time). [3]

[2] this bug
[3] bug 77941

Comment 3

[Chris Waterson]

Okay, so it turns out we *don't* need the text color for reflow (surprise!) and 
the code that's retrieving this information is doing so to color the selection. 
We can certainly fetch this information lazily.

Comment 4

[Chris Waterson]

Attachment: don't fetch selection colors until needed, diff -wu

Comment 5

[Chris Waterson]

cc'ing pierre, who put the calls to nsILookAndFeel in (r1.231), and mjudge, who 
is familiar with the selection code. The above patch avoids fetching the 
selection color from nsILookAndFeel until someone actually wants to draw with 
it.

Comment 6

[Chris Waterson]

Attachment: same patch, diff -u

Comment 7

[rbs]

Patch looks good - InitSelectionColors() is deferred until the last minute.
Reading bug 77941 however, I am wondering if this isn't really the one that 
should be fixed. It is kinda weird to do this trick here for something that 
should be O(1) if bug 77941 was handled properly. Is it unlikely that bug 77941 
is going to be fixed RSN?

Comment 8

[Chris Waterson]

True. I dread fixing bug 77941 on all three platforms (which is a lame excuse), 
but I also wonder if there may be other text style information that we might not 
need to fetch so eagerly?

Comment 9

[rbs]

[mid-air collision, re-submitting...]
Another alternative of optimization here might be to instead pass a  boolean 
(displaySelection & isSelected) to the ctor of nsTextStyle and if'ing the 
execution of the hot spot unless the boolean is set. Reading the code of 
nsTextFrame, it seems this is going to only happen in some Paint() if/when the 
colors are needed.

Comment 10

[rbs]

Does you lazy trick helped performance-wise to put the ctor off the radar? If
the patch helped, then maybe there could be other bigger fish elsewhere needing 
more worry than this one. I would favor nailing bug 77941 as the next step :-)

The problem with TextStyle is that it is trying to hold on to many things.
One would think that there should have been little helper structs like
TextStyleFont, TextStyleColor (somewhat similar to what the style system does).
But nsTextFrame is deliberately avoiding that and is agregating anything style 
related in its TextStyle so as to easily call its various rendering routines.
Side-effects of this heavy aggregation include the fact that the ctor is big and 
all users pay the price even if not all the things being initialized are 
relevant to them.

Some of the initializations there are nevertheless pretty straightforward.
The only other spot that caught my attention was the following (GetMetricsFor
and SetFont, and the various addref/release next and in the chain):

      nsCOMPtr<nsIDeviceContext> deviceContext;
      aRenderingContext.GetDeviceContext(*getter_AddRefs(deviceContext));
      nsCOMPtr<nsIAtom> langGroup;
      if (mDisplay->mLanguage) {
        mDisplay->mLanguage->GetLanguageGroup(getter_AddRefs(langGroup));
      }
      deviceContext->GetMetricsFor(*plainFont, langGroup, mNormalFont);
      aRenderingContext.SetFont(mNormalFont);
      aRenderingContext.GetWidth(' ', mSpaceWidth);

Comment 11

[Pierre Saslawsky]

Oh my!  It's clear that nsStyleColor has nothing to do with reflow, just 
rendering.  It could be a significant improvement in how the style system is 
used.  The structure which is the most used overall is Display but Color comes 
next and then Text, Position and Font.  If that's of any indication, 4 out of 5 
of these structures are in the Layout TextStyle structure.

Comment 12

[Chris Waterson]

*** Bug 90872 has been marked as a duplicate of this bug. ***

Comment 13

[Daniel Bratell]

I knew I had seen this somewhere! Now I just have to mention GetSysColor so that 
I find this bug next time I'm planning on filing a dupe. :-)

I have an idea for a different fix. Why not skip the color styles in the 
nsTextStyle alltogether and instead of move them to another struct, 
nsTextStyleWithColor that inherits from nsTextStyle. Then functions could use 
nsTextStyle when they don't care about colors (reflow) and nsTextStyleWithColor 
when they care (painting).

I guess the code changes would be small and you wouldn't have to add getter 
functions, the flag and the lazy code.

Comment 14

[Daniel Bratell]

Maybe a bad idea, that would still leave expensive selection color fetching even 
though those colors aren't needed even for normal painting.

Comment 15

[rbs]

This bug became moot ever since bug 77941 was fixed, I stepped in the debugger 
and nsLookAndFeel::GetColor() is now blazing fast...

What might be more interesting, re: other properties that may be lazily fetched, 
is perhaps having a general mechanism "compute-on-read" for the critical
properties. But that would be some work.

Indeed, the other things that are happening in the ctor of TextStyle:
      // Get style data
      mColor = (const nsStyleColor*) sc->GetStyleData(eStyleStruct_Color);
      mFont = (const nsStyleFont*) sc->GetStyleData(eStyleStruct_Font);
      mText = (const nsStyleText*) sc->GetStyleData(eStyleStruct_Text);
      mVisibility = (constnsStyleVisibility*)
        sc->GetStyleData(eStyleStruct_Visibility);

These defeat the optimizations in the Style System (given that the Style System
works in a kinda compute-on-read manner). Now I see where the rest of the '45%' 
taken by the ctor of TextStyle may come from... Suggesting, e.g., having 
m[Color|...]Data with an overloaded [->] so that:

mColorData[->]x { 
  if (has not yet computed realColor) compute realColor;
  forward [->] to realColor->x; // this easier said than done :-(
}

Comment 16

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comments from bug 116437, which is about a 24MB file taking forever to layout.
The jprof is at http://bugzilla.mozilla.org/showattachment.cgi?attach_id=65191
 
Typical: 
  Mostly Reflowing lines:

12% direct (90% total) in ReflowDirtyLines()
4% total in nsFontCache::GetMetricsFor()
6% direct in nsBlockReflowState::RecoverStateFrom()
2% direct in nsBlockFrame::PropagateFloaterDamage()
3% total in nsFrame::Invalidate()
10% direct in nsLineBox::GetCombinedArea()
4% direct, 13% total in nsBlockFrame::ComputeFinalSize()
6% direct in nsBlockFrame::BuildFloaterList()
8% total in nsBlockFrame::PlaceLine()
1.5% total in nsCSSRendering::FindBackground
3% total in nsHTMLReflowState::nsHTMLReflowState
5% direct in nsFrameList::LastChild
10% total in nsRenderingContextGTK::GetTextDimensions()

Comment 18

[rbs]

> 10% total in nsRenderingContextGTK::GetTextDimensions()

Linux people, it might be time to bite the bullet and try the version with break
indices. Switching to ResolveForward() (from bug 99010) will now ease the
migration. The folks of OS/2 did that. Also, this is becoming a matter of
necessity seing the support of plane-1 Unicode characters (UCS4) being added in
bug 118000.

Comment 19

[Jerry]

what's the status of this bug?

Comment 20

[rbs]

This bug is just noise now given that bug 77941 has been fixed.

Comment 21

[Jerry]

I dont understand what is noise to this bug.
bug 77941 has been fixed, getColor will be more efficiently.
but this bug is about to remove Text color from textframe's reflow.

Comment 22

[rbs]

It used to be important but it is just noise now. To get the color is essentially
O(1) now. So avoiding to look the color in nsTextFrame() is just an academic
exercise at the price of further complications in an already complex file. What
seems more challenging is to use the Style System efficiently (comment #11).

Comment 23

[Markus Hübner]

Any news / investigations in the meantime concerning comment #11 ?
Sound very promising.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So... perhaps the TextStyle should just hold a ref to the style context?  Then
it could get style structs from there on demand.

If we don't mind updating the callers, this would be trivial to do....

Comment 25

[rbs]

I am marking this as a dup of bug 244651. Rather than using a complicated
algorithmic-based approach (as seen, e.g., in attachment 32532 [details] [diff] [review] here), bryner had
the nice idea of simply splitting the nsTextStyle object into two: one used for
reflow (no color here), and an additional one for paint (with color). This
solved the problem neatly.

*** This bug has been marked as a duplicate of 244651 ***