1566945 - pass line box through from nsTextFrame::SetLength to nsBlockFrame::AddFrames

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, task)

Description

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

This is closely related to both bug 1561394 and bug 1300293 and some of the other related bugs -- but it only fixes parts of some of them, so I'm filing it as its own bug.

In particular, this fixes the RFindLineContaining part of the profile from bug 1561394 (which covers actual plaintext pages) but doesn't appear to fix view-source pages (which have a lot of markup in them and aren't really plaintext). But it also doesn't fix the nsTextFrame::GetInFlowContentLength part of bug 1561394.

The point of these patches is to pass the line box through a few stack frames so that we don't need to find it again and can avoid calling RFindLineContaining.

(It's possible a similar approach could be used for bug 1300293, but I still need to look into that.)

Comment 1

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

• profile of view-source of html spec: in nightly, with patches
• profile of a raw log for a linux debug build on mozilla-central treeherder: in nightly, with patches

Comment 2

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

Here's a try run for this set of patches.

Comment 3

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

Attachment: Bug 1566945 - Add a parameter to nsContainerFrame::InsertFrames for aPrevFrame's line box.

For now, always pass null, except when passing it through from one
overload to another.

Comment 4

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

Attachment: Bug 1566945 - Pass line box through from nsTextFrame::SetLength.

Comment 5

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

Attachment: Bug 1566945 - Pass line box through to nsBlockFrame::AddFrames.

Comment 6

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

Attachment: Bug 1566945 - Make nsBlockFrame::AddFrames not search for the line box if it is provided.

Comment 7

[Daniel Holbert [:dholbert]]

I wonder if this will help with bug 1456964 and bug 1475222, which are both about excessive time spent in RFindLineContaining...

Like those bugs, let's track this under [qf] to call out the perf win here. I'm calling this [qf:p2:responsiveness] for now (copied from the former bug), though if we have reason to believe there's a pageload impact, we can bump that to [qf:p1:pageload] (copied from the latter bug).

Comment 8

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

Given that addressing review comments involved adding new assertions, here's a new try run.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb32f80c5c9d
Add a parameter to nsContainerFrame::InsertFrames for aPrevFrame's line box. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/55569be3dc17
Pass line box through from nsTextFrame::SetLength. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/92d38cae0841
Pass line box through to nsBlockFrame::AddFrames. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4116a7254a4e
Make nsBlockFrame::AddFrames not search for the line box if it is provided. r=dholbert

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bb32f80c5c9d
https://hg.mozilla.org/mozilla-central/rev/55569be3dc17
https://hg.mozilla.org/mozilla-central/rev/92d38cae0841
https://hg.mozilla.org/mozilla-central/rev/4116a7254a4e