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

Optimize segment builder no-ops. #3273

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Nov 5, 2018

If we don't end up adding any clip regions to the segment builder,
then there is no point in going through the process of creating
events, sorting, and emitting segments.

Instead, detect this case and early out with a single segment.

This takes the backend time on the displaylist_mutate test on
my machine from 10.8ms to 8.2ms, which is quite a significant
improvement.


This change is Reviewable

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 5, 2018

r? anyone

@gw3583

This comment has been minimized.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 6, 2018

Try run looks good.

@nical

nical approved these changes Nov 6, 2018

// First, filter out any items that don't intersect
// with the visible bounding rect.
self.items.retain(|item| item.rect.intersects(&bounding_rect));
if self.has_interesting_clips {

This comment has been minimized.

@nical

nical Nov 6, 2018

Collaborator

nit: I have a slight preference for avoiding right drift by doing:

if early_out_cond {
    // early out code
}

// most of the stuff

I think that avoiding lots of nested scope makes the code generally easier to read and less intimidating.

This comment has been minimized.

@gw3583

gw3583 Nov 6, 2018

Collaborator

In general I agree with you. I didn't do that in this case because there is a piece of code at the end of the method (that resets the initialization state) that much run regardless of which branch is taken. Although, having said that, I just realized that can be moved above the early out in this case.

This comment has been minimized.

@gw3583

gw3583 Nov 6, 2018

Collaborator

Pushed an update to resolve this.

Optimize segment builder no-ops.
If we don't end up adding any clip regions to the segment builder,
then there is no point in going through the process of creating
events, sorting, and emitting segments.

Instead, detect this case and early out with a single segment.

This takes the backend time on the displaylist_mutate test on
my machine from 10.8ms to 8.2ms, which is quite a significant
improvement.
@nical

This comment has been minimized.

Collaborator

nical commented Nov 6, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 6, 2018

📌 Commit b9f81de has been approved by nical

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 6, 2018

⌛️ Testing commit b9f81de with merge 4914768...

bors-servo added a commit that referenced this pull request Nov 6, 2018

Auto merge of #3273 - gw3583:seg-opt, r=nical
Optimize segment builder no-ops.

If we don't end up adding any clip regions to the segment builder,
then there is no point in going through the process of creating
events, sorting, and emitting segments.

Instead, detect this case and early out with a single segment.

This takes the backend time on the displaylist_mutate test on
my machine from 10.8ms to 8.2ms, which is quite a significant
improvement.

<!-- 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/3273)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 6, 2018

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

@bors-servo bors-servo merged commit b9f81de into servo:master Nov 6, 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