1645713 - FindNearestCommonAncestor does more work than BuildTextRunsScanner::ContinueTextRunAcrossFrames needs

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Description

[Cameron McCormack (:heycam)]

Profiling a page with a lot of text, I find 2.5% of Reflow time spent under nsLayoutUtils::FindNearestCommonAncestor. That function loops all the way up to the root frame for the two arguments, but for its use in BuildTextRunsScanner::ContinueTextRunAcrossFrames, we know that the two text frames are within the same block. We can add a variation of FindNearestCommonAncestor that only looks up to the closest block.

https://share.firefox.dev/3d5N990

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bac53350867278e4caa0150fe3622f25cfe6a8

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1645713 - Do less work when finding common ancestor for text run creation.

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28f4fadf27c1bf6a59b2e25147f80a8ec4f41018

Comment 4

[Cameron McCormack (:heycam)]

There's a case that this patch crashes on, due to the two text frames being in different blocks as they're fragmented across columns: layout/base/crashtests/538267-1.html. It means that we return null from FindNearestCommonAncestorFrameWithinBlock and then we assert. It seems like it should be safe to return false from ContinueTextRunAcrossFrames in this case, rather than trying harder to find a common ancestor to check styles up the tree. Does that sound right, Jonathan?

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a9fa3a8b7dbd651774ed30fb4558920d9cd9f80

Comment 6

[Jonathan Kew [:jfkthame]]

There's a case that this patch crashes on, due to the two text frames being in different blocks as they're fragmented across columns

Ugh... should've known there'd be some case that would complicate life! That suggests the same thing can happen across a page break, too.

In theory, we shouldn't allow that to interrupt text run creation, as there could be contextual shaping effects that depend on context and should be maintained even across the break; but I think that's such an obscure edge case that it's OK to ignore it for now. In almost all cases, interrupting shaping will be the right thing to do, and I'm not even sure how easily we'd be able to tell if it wasn't. (We have more serious issues to worry about regarding how line-breaks interact with shaping, anyhow.)

Please leave a comment in the code noting this case, though, to remind us if/when we ever revisit this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac246edd1e36
Do less work when finding common ancestor for text run creation. r=jfkthame

Comment 8

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ac246edd1e36