1535134 - nsLayoutUtils::DisplayRootHasRetainedDisplayListBuilder is slow

Status: NEW

(Core :: Web Painting, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

When rendering the single-page HTML spec, we spent 53.4ms inside the boolean function nsLayoutUtils::DisplayRootHasRetainedDisplayListBuilder. That seems high and like something we perhaps can optimize.

Here's a profile, filtered for this function: https://perfht.ml/2SZRxMV

This function is only 2 lines long (ignoring the debug-only MOZ_ASSERT statement):

bool nsLayoutUtils::DisplayRootHasRetainedDisplayListBuilder(nsIFrame* aFrame) {
  const nsIFrame* displayRoot = nsLayoutUtils::GetDisplayRootFrame(aFrame);
  MOZ_ASSERT(displayRoot);
  return displayRoot->HasProperty(RetainedDisplayListBuilder::Cached());
}

I don't know offhand which line is the more-expensive one, or if they're both expensive. But notably, the profiler does show that this function spends some of its time (5.6ms) in mozilla::ViewportFrame::GetViewInternal, which I think is somewhere inside of the call to GetDisplayRootFrame. (It seems that intervening stack levels have been inlined so I don't know the exact callpath.)

Comment 1

[Daniel Holbert [:dholbert]]

Searchfox link for this function:
https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/layout/base/nsLayoutUtils.cpp#478-481

Comment 2

[Daniel Holbert [:dholbert]]

It's conceivable that HasProperty() might be slow, if the root frame has lots of frame properties set (because the property table is a linked list which must be traversed to answer the "HasProperty" question).

If we had a spare generic frame state bit, we could optimize that call away by using a frame state bit as a proxy for whether the property is set or not.

Comment 3

[Daniel Holbert [:dholbert]]

(Or I guess we could just a bool mFoo : 1; bitfield member-var on the frame. I'm giving that a shot to see if it helps here.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: strawman patch to use a bool instead of HasProperty() check (probably not worth landing)

I tried a patch to use a bool : 1 instead of the HasProperty() check, but it didn't help much.

Methodology: I tested a local build (--enable-optimize --enable-debug-symbols), with this patch applied, and I compared the the "optimized" bool-check (the version in this patch) vs. the same build after restoring the original HasProperty(...) check in nsLayoutUtils.cpp, and I got these measurements:

Using bool member-var:
30.6ms https://perfht.ml/2XWu5DS
29.8ms https://perfht.ml/2XULz3w
26.4ms https://perfht.ml/2T15j1z
28.6ms https://perfht.ml/2T4GbqG

Using HasProperty:
42.8ms https://perfht.ml/2XY5FJZ
30.4ms https://perfht.ml/2Y4LF8S
27.8ms https://perfht.ml/2XWujLl
31.4ms https://perfht.ml/2XSA1xP

Aside from the first 42.8ms measurement with HasProperty (which may've been a fluke/outlier), the measurements are all pretty equivalent, so this strawman patch doesn't seem to help much.

That leads me to believe that DisplayRootHasRetainedDisplayListBuilder's other line (nsLayoutUtils::GetDisplayRootFrame) is the slow thing here.

Comment 5

[Daniel Holbert [:dholbert]]

[er, didn't intend to take bug necessarily; i forgot to untick the 'assign-to-me' checkbox that activates when posting a patch]

Comment 6

[Markus Stange [:mstange]]

Matt, are we close to having retained display lists everywhere? Would that allow us to eliminate the check entirely?

Comment 7

[Miko Mynttinen]

I think the main problem here is that we mark all reflowed frames modified. I think that this is not necessary, and only marking reflow root modified should be enough. Then we could add a check here1, and skip all the frames that are in reflow.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Miko Mynttinen [:miko] from comment #7)

I think the main problem here is that we mark all reflowed frames modified. I think that this is not necessary, and only marking reflow root modified should be enough. Then we could add a check here1, and skip all the frames that are in reflow.

This is true for the initial reflow when loading, but not for normal reflows. Usually when we reflow for a small change, most frames aren't modified, and we don't want to mark the whole frame tree under the reflow root as modified.

(In reply to Markus Stange [:mstange] from comment #6)

Matt, are we close to having retained display lists everywhere? Would that allow us to eliminate the check entirely?

Timothy is working on it!

I'm also not sure why we need to check the root frame for the presence of a retained DL builder, when we could just check the pref value. Seems like there could be a lower overhead way to determine this.

Comment 9

[Miko Mynttinen]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)

I'm also not sure why we need to check the root frame for the presence of a retained DL builder, when we could just check the pref value. Seems like there could be a lower overhead way to determine this.

I looked into this a bit more. We have a couple of cases where just checking the pref is not enough:

1. Hit testing
2. nsLayoutUtils::PaintFrame() called for non-root frame
3. Most calls to nsLayoutUtils::PaintFrame() through PresShell::RenderDocument()