1425863 - Make fewer nsDisplayCompositorHitTestInfo display items

Status: RESOLVED DUPLICATE of bug 1534549

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Right now we create a lot of nsDisplayCompositorHitTestInfo items (one per frame, almost). When I originally added this code I intentionally wrote it in such a way that instead of creating dedicated items for each frame, we could piggyback the actual hit-test flags on other display items (e.g. a background display item for the frame). We should do that as it will help a lot in terms of reducing display list size and processing time.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

The other thing we can do to create fewer of these is to skip the ones where the rect is contained inside another nsDisplayCompositorHitTestInfo with a matching ASR and hit info.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Are there plans to unify nsDisplayLayerEventRegions and nsDisplayCompositor HitTestInfo (and even nsDisplayList::HitTest)?

Would be nice if we could have a single codepath for all our hit-testing needs.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I agree that it would be nice to unify all the things. I have thought a little about removing nsDisplayLayerEventRegions and using the nsDisplayCompositorHitTestInfo instead, and couldn't think of any blockers except possibly performance but I think we should be able to make it comparable. I haven't thought at all about unifying nsDisplayList::HitTest though.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Should we close this now that part 4 of bug 1434243 has landed?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I think we can still leave it open. Part 4 of bug 1434243 implements comment 1, but we can still do the thing from comment 0. That will also further unify the main-thread gecko hit-testing with the compositor hit-testing codepaths.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

We don't need to block shipping on this unless we need the extra performance.

Comment 7

[Miko Mynttinen]

I have a WIP patch that makes nsDisplayWrapList carry hit test information in some cases. I have been testing this on Facebook, where nsDisplayCompositorHitTestInfo items account for almost 30% of total display items.

The patch currently implements this for FrameLayerBuilder, but the logic should be very similar for WebRender. I am currently investigating whether the performance improvements from this change make the patch worth landing, currently the improvements seem minimal.

Comment 8

[Miko Mynttinen]

Attachment: 1425863.patch

Comment 9

[Kartikaya Gupta (email:[e-mail])]

It would be good to split this into another bug since I had filed this bug for a very specific optimization I wanted to do, as described in comment 0 (it's specific to WR).

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Miko, is it worth keeping this bug open still?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I would still like to keep it open until I have a chance to try the thing from comment 0.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Talked this over with Matt a bit to get a better understanding of how to implement this change. The general idea is that if a frame adds a background color item here then we should make that background item's WR commands carry the hit-test info to WebRender. If the frame doesn't create a background color item, then it can instead add a nsDisplayCompositorHitTestInfo item to the background list, and that would carry the hit-test info to WebRender.

There will be cases where we end up creating spurious nsDisplayCompositorHitTestInfo items, but we can at least try and optimize away the corresponding WR dummy rects by checking if they are contained inside the previous rect with hit-test info.

Comment 13

[Miko Mynttinen]

I implemented this in bug 1534549.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Awesome, thank you! I'm looking forward to hopefully seeing some good perf improvement numbers :)