New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a recent bug which resulted in redundant clips being drawn. #2986

Merged
merged 1 commit into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Contributor

gw3583 commented Aug 27, 2018

This fixes a performance regression where primitives with
segments would allocate and draw clip masks for the edge
segments.

It's probably not a noticeable regression on most sites.

Also updated rounded rects reftest to have one primitive which
is large enough to invoke clip segment logic.


This change is Reviewable

Fix a recent bug which resulted in redundant clips being drawn.
This fixes a performance regression where primitives with
segments would allocate and draw clip masks for the edge
segments.

It's probably not a noticeable regression on most sites.

Also updated rounded rects reftest to have one primitive which
is large enough to invoke clip segment logic.
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 27, 2018

Contributor

r? @kvark

Contributor

gw3583 commented Aug 27, 2018

r? @kvark

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583
Contributor

gw3583 commented Aug 27, 2018

@@ -2211,7 +2211,8 @@ impl Primitive {
match segment_clip_chain {
Some(segment_clip_chain) => {
if segment_clip_chain.clips_range.count == 0 {
if segment_clip_chain.clips_range.count == 0 ||
(!segment.may_need_clip_mask && !segment_clip_chain.has_non_local_clips) {

This comment has been minimized.

@kvark

kvark Aug 28, 2018

Member

hmm, shouldn't this be segment_clip_chain.clips_range.count == 0 || !segment.may_need_clip_mask || segment_clip_chain.has_non_local_clips?

@kvark

kvark Aug 28, 2018

Member

hmm, shouldn't this be segment_clip_chain.clips_range.count == 0 || !segment.may_need_clip_mask || segment_clip_chain.has_non_local_clips?

This comment has been minimized.

@gw3583

gw3583 Aug 28, 2018

Contributor

I don't think so.

The segment.may_need_clip_mask field (perhaps confusingly) refers to whether that segment might need a clip mask due to it's local segment.

So this condition says a segment can be drawn opaque if there are no relevant clip masks at all, or if the segment builder didn't have any local clips and there's no global clips that affect this segment.

It is subtle though, so I could be making a mistake here - could you explain your reasoning why it should be the above?

@gw3583

gw3583 Aug 28, 2018

Contributor

I don't think so.

The segment.may_need_clip_mask field (perhaps confusingly) refers to whether that segment might need a clip mask due to it's local segment.

So this condition says a segment can be drawn opaque if there are no relevant clip masks at all, or if the segment builder didn't have any local clips and there's no global clips that affect this segment.

It is subtle though, so I could be making a mistake here - could you explain your reasoning why it should be the above?

This comment has been minimized.

@kvark

kvark Aug 28, 2018

Member

I think may_need_clip_mask just needs to be renamed for clarity. The code is good.

@kvark

kvark Aug 28, 2018

Member

I think may_need_clip_mask just needs to be renamed for clarity. The code is good.

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Aug 28, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 28, 2018

Contributor

📌 Commit 14d8157 has been approved by kvark

Contributor

bors-servo commented Aug 28, 2018

📌 Commit 14d8157 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 28, 2018

Contributor

⌛️ Testing commit 14d8157 with merge 8fd8a5b...

Contributor

bors-servo commented Aug 28, 2018

⌛️ Testing commit 14d8157 with merge 8fd8a5b...

bors-servo added a commit that referenced this pull request Aug 28, 2018

Auto merge of #2986 - gw3583:clip-seg-opt, r=kvark
Fix a recent bug which resulted in redundant clips being drawn.

This fixes a performance regression where primitives with
segments would allocate and draw clip masks for the edge
segments.

It's probably not a noticeable regression on most sites.

Also updated rounded rects reftest to have one primitive which
is large enough to invoke clip segment logic.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2986)
<!-- Reviewable:end -->
@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Aug 28, 2018

Member

@bors-servo r-
hmm, what about those R1 failures?

Member

kvark commented Aug 28, 2018

@bors-servo r-
hmm, what about those R1 failures?

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 28, 2018

Contributor

Those R1 failures are expected due to a previous PR - they need a slight bit of fuzziness added. I think that has actually already landed now, it's just that this try run was done before that WR update landed.

Contributor

gw3583 commented Aug 28, 2018

Those R1 failures are expected due to a previous PR - they need a slight bit of fuzziness added. I think that has actually already landed now, it's just that this try run was done before that WR update landed.

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Aug 28, 2018

Member

The windows R1 isn't a matter of fuzz:

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1535339481/build/tests/reftest/tests/layout/reftests/bugs/403181-1.xml == file:///C:/Users/task_1535339481/build/tests/reftest/tests/layout/reftests/bugs/403181-1-ref.xml | image comparison, max difference: 255, number of differing pixels: 900

Member

kvark commented Aug 28, 2018

The windows R1 isn't a matter of fuzz:

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/Users/task_1535339481/build/tests/reftest/tests/layout/reftests/bugs/403181-1.xml == file:///C:/Users/task_1535339481/build/tests/reftest/tests/layout/reftests/bugs/403181-1-ref.xml | image comparison, max difference: 255, number of differing pixels: 900

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 28, 2018

Contributor

@kvark Ah well spotted - I missed that. Let me take a look now.

Contributor

gw3583 commented Aug 28, 2018

@kvark Ah well spotted - I missed that. Let me take a look now.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 28, 2018

Contributor

OK, it looks like a know intermittent failure - https://bugzilla.mozilla.org/show_bug.cgi?id=1455192. However, I have re-triggered that build to make sure.

Contributor

gw3583 commented Aug 28, 2018

OK, it looks like a know intermittent failure - https://bugzilla.mozilla.org/show_bug.cgi?id=1455192. However, I have re-triggered that build to make sure.

@kvark

kvark approved these changes Aug 28, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 28, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
State: approved= try=False

Contributor

bors-servo commented Aug 28, 2018

☀️ Test successful - status-appveyor, status-taskcluster
State: approved= try=False

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 28, 2018

Contributor

R1 passed in the re-triggered try.

@bors-servo r=kvark

Contributor

gw3583 commented Aug 28, 2018

R1 passed in the re-triggered try.

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 28, 2018

Contributor

📌 Commit 14d8157 has been approved by kvark

Contributor

bors-servo commented Aug 28, 2018

📌 Commit 14d8157 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 28, 2018

Contributor

⌛️ Testing commit 14d8157 with merge def6599...

Contributor

bors-servo commented Aug 28, 2018

⌛️ Testing commit 14d8157 with merge def6599...

bors-servo added a commit that referenced this pull request Aug 28, 2018

Auto merge of #2986 - gw3583:clip-seg-opt, r=kvark
Fix a recent bug which resulted in redundant clips being drawn.

This fixes a performance regression where primitives with
segments would allocate and draw clip masks for the edge
segments.

It's probably not a noticeable regression on most sites.

Also updated rounded rects reftest to have one primitive which
is large enough to invoke clip segment logic.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2986)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 29, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing def6599 to master...

Contributor

bors-servo commented Aug 29, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing def6599 to master...

@bors-servo bors-servo merged commit 14d8157 into servo:master Aug 29, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment