864129 - make consumers of nsHTMLReflowState not use its cached style struct members

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

Per bug 861746 comment 6, there shouldn't be a need for users of nsHTMLReflowState to grab its cached style struct pointers; they can instead get them off the frame.

Comment 1

[Cameron McCormack (:heycam)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=c630b65f2b1b

Comment 2

[Boris Zbarsky [:bzbarsky]]

It used to be this was done for performance, but maybe the getters on the frame are faster now...

Comment 3

[Mats Palmgren (inactive)]

Fwiw, I hacked up a quick microbenchmark for this, which showed:
state->mStyleVisibility->mDirection vs.
state->frame->StyleVisibility()->mDirection
the latter is 6 times slower

state->mStyleDisplay->mPosition vs.
state->frame->StyleDisplay()->mPosition
the latter is 11 times slower

DISCLAIMER: totally unscientific test, so you should double-check...

Comment 4

[Daniel Holbert [:dholbert]]

Hmm, OK - I suppose that makes sense.  I had a feeling that might be the case, but didn't verbalize it in the comment over on bug 861746 -- sorry about a sending you down an possibly-unwanted rabbit hole. :-/

So -- assuming that there's a perf benefit to using these cached pointers, we probably should be using them wherever possible (i.e. whenever we have a reflow state available), instead of grabbing them off the frame.  I'm pretty sure that there are a lot of places where we could be using them but don't, right now.  (That change wouldn't be a prereq for bug 861746, but would be worth pursuing at some point.)

Comment 5

[Cameron McCormack (:heycam)]

It might be that the slowness of calling the style struct getters isn't a problem in practice.  Do we trust talos enough to just see if there's a performance regression there?  (If not, should we be having regression tests for these performance optimisations that we "know" are important but which aren't tested by talos?)  I'll await dbaron's views...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

comment 3 has more useful data than I do.  I'm reasonably sure it was done for performance, and my guess is that it's probably not worth spending the energy to figure out how useful it is.

Comment 7

[Daniel Holbert [:dholbert]]

FWIW, we do frequently go to the trouble of avoiding repeated calls to a given StyleFoo() getter within a given function, e.g.
> 139         const nsStyleDisplay* display = frame->StyleDisplay();
> 140         if (display->IsBlockOutsideStyle() ||
> 141             display->mDisplay == NS_STYLE_DISPLAY_TABLE_CELL) {
https://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsTextEquivUtils.cpp#139

If that's worth doing, then it seems like caching the pointer on the nsHTMLReflowState is even more worth doing - it's basically the same sort of optimization, but with a larger scope. (but with the lifetime still scoped to the lifetime of the nsHTMLReflowState, i.e. the duration of the reflow)

Bottom line: I think I led you astray in suggesting that you file/fix this bug. (Sorry about that :-/ ).  I think we should WONTFIX this bug and keep the nsHTMLReflowState::StyleFoo() getters that you'd initially added in bug 861746.

Comment 8

[Cameron McCormack (:heycam)]

OK, let's WONTFIX.  For posterity, I did a try run with and without the patch.  Here's the talos comparison: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b1300ab43ae7&newRev=9d0f630b2e06&submit=true

It's difficult for me to read anything from that without knowing how reliable the talos results are.  An 81.29% Windows 7 tsvr_opacity regression seems unlikely, for example.