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.

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

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

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

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

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

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.

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.