902701 - Use nsHTMLReflowState's cached style struct pointers more aggressively

Status: RESOLVED INACTIVE

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

nsHTMLReflowState goes to the trouble of caching pointers for various style structs from its frame, in ::Init, here:

287 void
288 nsHTMLReflowState::Init(nsPresContext* aPresContext,
289                         nscoord         aContainingBlockWidth,
290                         nscoord         aContainingBlockHeight,
291                         const nsMargin* aBorder,
292                         const nsMargin* aPadding)
293 {
294   NS_WARN_IF_FALSE(availableWidth != NS_UNCONSTRAINEDSIZE,
295                    "have unconstrained width; this should only result from "
296                    "very large sizes, not attempts at intrinsic width "
297                    "calculation");
298 
299   mStylePosition = frame->StylePosition();
300   mStyleDisplay = frame->StyleDisplay();
301   mStyleVisibility = frame->StyleVisibility();
302   mStyleBorder = frame->StyleBorder();
303   mStyleMargin = frame->StyleMargin();
304   mStylePadding = frame->StylePadding();
305   mStyleText = frame->StyleText();

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=556a557c7276#287

...but then in many cases in Reflow, we don't bother using those cached pointers -- we'll re-invoke StylePosition() even though we have the reflowState for the same frame still in-scope. (or even if we *are* the reflow-state and have those cached as this->mStyleWhatever.)

In bug 864129 comment 3, mats indicated (with some data) that there's a nontrivial cost associated with unnecessary invocations of StylePosition() & friends. So, I'm filing this bug on more aggressively reusing the cached style struct pointers.

Here are a few instances of this that I just found in nsHTMLReflowState itself:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=556a557c7276#2160
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=556a557c7276#2433
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=556a557c7276#2455
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=556a557c7276#2472

Comment 1

[Arnaud Sourioux [:Six]]

I'm gonna work on it for the next days,
i will catch you on irc if i need specific details but it looks okay.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks!

(There may be some cases where this is a little tricky -- in particular, be careful not to switch from frame->StyleWhatever() to nsHTMLReflowState::mStyleWhatever when the reflow state is for a different frame.)

Comment 3

[Daniel Holbert [:dholbert]]

Darn... as Six points out in IRC, the URLs in comment 0 are all ineligible for this conversion -- they're for nsCSSOffsetState methods, and nsCSSOffsetState doesn't have these pointers.

Here are two that should be eligible, though:
> 1549   const nsStyleText* styleText = StyleText();
> 1550   const nsStyleTextReset* styleTextReset = StyleTextReset();
> 1551   // See if we can try and avoid marking all the lines as dirty
> 1552   bool tryAndSkipLines =
> 1553     // The block must be LTR (bug 806284)
> 1554     StyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR &&
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=f10d31a0f7b4#1549

> 4058   const nsStyleText* styleText = StyleText();
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=f10d31a0f7b4#4058

The StyleText() and StyleVisibility() invocations there can be replaced.

We have a "nsBlockReflowState& aState" in-scope, which has a "mReflowState" pointer for this frame, which has cached mStyleText and mStyleVisibility pointers.

So we should be able to swap out the StyleText() calls and the StyleVisibility() call with aState.mReflowState.mStyleText and aState.mReflowState.mStyleVisibility.

(We probably want to keep the local-variable "styleText" alias, since we use it several times, and it's nice not to have the clutter of "aState.mReflowState" every time. Plus it'll help keep the patch more targeted.)

Comment 4

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 5

[Daniel Holbert [:dholbert]]

This isn't really worth tracking at this point. There are potentially minor wins to be had from using these precomputed pointers, but I think the performance implications have changed with Stylo and are maybe less worth worrying about nowadays.

We can make incremental fixes along the lines of this bug, in one-off bugs, if anyone cares to do so.