Closed Bug 1342801 Opened 7 years ago Closed 7 years ago

Store the 'writing-mode' used value on nsIFrame and make GetWritingMode non-virtual.

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

I saw someone comment that GetWritingMode calls can be 1-2% of reflow intensive
pages, and I've seen numbers in that region too.  I suspect this is mostly due
to the virtual dispatch, but perhaps also some cache pressure from constantly
accessing the style value, and recalculating the used value from that.

We can fix all those problems by calculating it once for the lifetime of
the frame and storing it in a field, and making GetWritingMode a trivial
non-virtual accessor method.

It just so happens that on 64-bit platforms (on Linux at least, but presumably
other platforms too) there's currently a 32-bit padding at the end of nsIFrame,
so adding a byte for mWritingMode there has zero cost.
Comment on attachment 8841443 [details] [diff] [review]
Store the 'writing-mode' used value on nsIFrame and make GetWritingMode non-virtual.

Review of attachment 8841443 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good optimization, thanks!

::: layout/generic/nsGfxScrollFrame.h
@@ +1073,5 @@
> +    mHelper.ReloadChildFrames();
> +    if (mHelper.mScrolledFrame) {
> +      mWritingMode = mHelper.mScrolledFrame->GetWritingMode();
> +    }
> +  }

Don't we need to use a similar wrapper for mHelper.ReloadChildFrames in nsXULScrollFrame as well?

Hmm, but we don't currently have a GetWritingMode override there, so maybe it's not needed. Or maybe that's a bug we should fix. Although we don't support vertical writing modes in XUL, we should support bidi, and the bidi direction value becomes part of what GetWritingMode returns. So unless there's a reason why the nsXULScrollFrame doesn't need this behavior, it kinda looks like we might have a bug that we just haven't noticed until now. WDYT?

::: layout/style/nsStyleStruct.cpp
@@ +3610,5 @@
>      // It's important that a change in mWritingMode results in frame
>      // reconstruction, because it may affect intrinsic size (see
>      // nsSubDocumentFrame::GetIntrinsicISize/BSize).
> +    // Also, the used writing-mode value is now a field on nsIFrame and some
> +    // classes (e.g. table rows/cells) copies their value from an ancestor.

s/copies/copy/

::: layout/tables/nsTableCellFrame.cpp
@@ +93,5 @@
>      cellFrame->GetColIndex(colIndex);
>      SetColIndex(colIndex);
> +  } else {
> +    // Although the spec doesn't say that writing-mode is not applied to
> +    // table-cells, we still override this method here because we want to

"we ... override this method here" isn't really the right phrase after moving this comment from the former GetWritingMode override.
Attachment #8841443 - Flags: review?(jfkthame) → review+
Most (all?) internal use of 'direction' is from ScrollFrameHelper::GetScrolledFrameDir()
or ScrollFrameHelper::IsPhysicalLTR() / IsBidiLTR(), which use ScrollFrameHelper::GetFrameForDir():
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/layout/generic/nsGfxScrollFrame.cpp#4944
So maybe GetWritingMode() isn't needed on the outer frame for 'direction' use?
Probably not worth bothering with unless you can think of something obvious that would fail...
Oh, I misread that, it seems GetWritingMode() is actually called on that:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/layout/generic/nsGfxScrollFrame.h#333
so yeah, that might be broken.  Anyway, that should go in a separate bug
since it would be a functional change (which this bug isn't).
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11781abf35b7
Store the 'writing-mode' used value on nsIFrame and make GetWritingMode non-virtual.  r=jfkthame
(In reply to Mats Palmgren (:mats) from comment #5)
> Oh, I misread that, it seems GetWritingMode() is actually called on that:
> http://searchfox.org/mozilla-central/rev/
> 4039fb4c5833706f6880763de216974e00ba096c/layout/generic/nsGfxScrollFrame.
> h#333
> so yeah, that might be broken.  Anyway, that should go in a separate bug
> since it would be a functional change (which this bug isn't).

Agreed. Would you mind filing a followup? (If you have time to look into it, great, but otherwise let's at least record the question.) Thanks!
No problem, filed bug 1343298.
https://hg.mozilla.org/mozilla-central/rev/11781abf35b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.