Skip clip masks where the inner rect encloses the primitive rect.

[glennw]

This fixes some performance issues in the Motionmark images demo.

---

This change is 

[glennw]

I'll run the Servo and Gecko tests against this before merging, but it seems like it should be a safe optimization.

r? @kvark

[glennw]

WPT/CSS tests in Servo pass.

Gecko try run here looks good (nearly complete):

https://hg.mozilla.org/try/rev/d0893ae2eb0a051db8819254054df695056dcf96
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0893ae2eb0a051db8819254054df695056dcf96

Fixes #1648

[kvark]

Thanks for tracking this down, @glennw !
I'm not fully convinced this hack is worth submitting. Another approach proposed in the comment.

[kvark]

There is only one case where task_rect is larger than prim_rect: when we are processing clips specified for parent stacking contexts, while the primitive itself doesn't have a specific clip.

But in this case it should be desirable to have this task even if the particular primitive rect is not affected, because the generated mask is to be re-used for multiple primitives inside this stacking context.

I think we need to sort this out at a higher level: have a heuristic for a stacking context to share it's mask with children primitives. The heuristic can be as follows:

1. compute the bounding box of all primitives inside this stacking context that don't have specific clips already
2. compute the sum of bounding box squares of all the same primitives
3. if the sum is smaller than the bounding box of all, then pass None here for clip_id and prim_rect for task_rect
4. if the sum is larger, then do what we are doing now

As you can see, with this approach we'll not need a hack inside new_mask, and we'll not pass the primitive rectangle in here (just the task rectangle).

[glennw]

That does sound a bit more elegant conceptually, but I think the implementation may be a bit complicated.

We'd have to either scan the primitive list inside a stacking context first to get the sum, and know which path to choose when generating the masks, or we'd have to keep track of the primitives that need a clip mask and then revisit them to set up the appropriate clip mask, wouldn't we? It's possible I'm over-thinking this :)

I think we could land this first, since it's a simple win, and then revisit the heuristic above as a possible follow up. What do you think?

[glennw]

Huh, I have an idea that might fix this and drastically simplify the way we manage shared masks. Will need some more thought and prototyping to see if it works though. @jrmuizel Do you want an urgent fix for that performance issue, or is it fine to hold off for a few days and work out the best long term solution?

[jrmuizel]

There's no urgency for a fix

…

On Aug 31, 2017 10:17 PM, "Glenn Watson" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In webrender/src/render_task.rs <#1649 (comment)>: > @@ -252,6 +253,20 @@ impl RenderTask { match clip_info.bounds.inner { Some(ref inner) if !inner.device_rect.is_empty() => { + // If this clip item has an inner rect that completely + // encloses the screen rect of the primitive, it can't + // possibly affect the result, so remove it during + // this filtering pass. + if inner.device_rect.contains_rect(&prim_rect) { Huh, I have an idea that might fix this and drastically simplify the way we manage shared masks. Will need some more thought and prototyping to see if it works though. @jrmuizel <https://github.com/jrmuizel> Do you want an urgent fix for that performance issue, or is it fine to hold off for a few days and work out the best long term solution? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1649 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUTbUZDGEk_TQh63FL96nnQ6FJ6d5p1ks5sd2kzgaJpZM4PJjKX> .

[glennw]

@kvark I wrote up what I've been thinking about for clip masks - https://github.com/servo/webrender/wiki/Improved-clipping.

When you have time available, it'd be great to hear any feedback or issues you can think of related to it. It seems like it should just work, but it's quite likely there's something subtle I haven't thought though...

[kvark]

@glennw Thank you! I moved the discussion to #1656

[glennw]

@kvark In terms of this PR / optimization - I realized a simpler way to achieve it. The patch now just skips creating a mask altogether if the final inner rect contains the primitive rect.

This removes any hacks regarding modifying the cache key, and still fixes up the performance of this benchmark.

Tests run OK in Servo, and Gecko try run here looks good:

https://hg.mozilla.org/try/rev/23927113bc7051c5779d3ed5c5dd5a0e16f2e689
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23927113bc7051c5779d3ed5c5dd5a0e16f2e689

(Also rebased).

[kvark]

hmm, so now of all the primitives sharing the same task, some will just cop out by returning None here and others will process as usual? seems logically correct to me

[glennw]

Yup, that's right :)

[kvark]

@bors-servo r+

[bors-servo]

📌 Commit c85571c has been approved by kvark

[bors-servo]

⌛ Testing commit c85571c with merge e5485fb...

[bors-servo]

☀️ Test successful - status-travis
Approved by: kvark
Pushing e5485fb to master...