Skip to content
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

Next set of ClipScrollTree optimizations #2156

Merged
merged 4 commits into from Dec 4, 2017

Conversation

@mrobinson
Copy link
Member

mrobinson commented Dec 2, 2017

This seems to cut the prominence of ClipScrollTree updates in my profile a great deal.


This change is Reviewable

@mrobinson mrobinson requested review from glennw and kvark Dec 2, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Dec 2, 2017

@glennw
Copy link
Member

glennw commented Dec 2, 2017

I'd like to get #2146 landed before we land this, if possible (doing a gecko try now, hopefully it will be ready after that), since they will conflict a bit.

This looks awesome from a quick scan though, it should help with CPU performance a lot!

@glennw
Copy link
Member

glennw commented Dec 4, 2017

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


webrender/src/prim_store.rs, line 1661 at r1 (raw file):

}

fn get_local_bounding_rect(

This uses transform_point3d while the version it's replaced with in util.rs uses transform_point2d. Perhaps that's not actually relevant here?


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Dec 4, 2017

Just one question, otherwise this looks good to me (it will need a rebase after #2146 lands too).

@bors-servo
Copy link
Contributor

bors-servo commented Dec 4, 2017

The latest upstream changes (presumably #2146) made this pull request unmergeable. Please resolve the merge conflicts.

mrobinson added 2 commits Dec 1, 2017
Rework ClipScrollTree::update, eliminated members which are now
duplicated in ClipScrollNodeData and reducing the amount of unnecessary
work. There are a few main thrusts here:

1. Don't do matrix operations if it's not necessary.
Transform3D::pre_mul was showing up in CPU profiles, and this makes it
much less significant.
2. Don't spend time setting up state if we are not going to recurse.
3. When we encounter an empty node (including those clipped out), do as
little as possible. This includes setting update state or calculating
things for ClipScrollNodeData.
Instead of duplicating the functionality of transform_rect in
get_local_bounding_rect, just use the function from utils.rs.
@mrobinson
Copy link
Member Author

mrobinson commented Dec 4, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/prim_store.rs, line 1661 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This uses transform_point3d while the version it's replaced with in util.rs uses transform_point2d. Perhaps that's not actually relevant here?

I think this should be okay. Both methods are using the same code to calculate the x and y coordinates of the final point.

    #[inline]
    pub fn transform_point3d(&self, p: &TypedPoint3D<T, Src>) -> TypedPoint3D<T, Dst> {
        let x = p.x * self.m11 + p.y * self.m21 + p.z * self.m31 + self.m41;
        let y = p.x * self.m12 + p.y * self.m22 + p.z * self.m32 + self.m42;
        let z = p.x * self.m13 + p.y * self.m23 + p.z * self.m33 + self.m43;
        let w = p.x * self.m14 + p.y * self.m24 + p.z * self.m34 + self.m44;

        point3(x/w, y/w, z/w)
    }
    #[inline]
    pub fn transform_point2d(&self, p: &TypedPoint2D<T, Src>) -> TypedPoint2D<T, Dst> {
        let x = p.x * self.m11 + p.y * self.m21 + self.m41;
        let y = p.x * self.m12 + p.y * self.m22 + self.m42;

        let w = p.x * self.m14 + p.y * self.m24 + self.m44;

        point2(x/w, y/w)
    }

transform_point2d should be slightly more efficient.


Comments from Reviewable

Instead of calculating some things that we no longer need for
TransformedRect, we simply calculate the screen bounding rect. We can
then completely remove TransformedRect. This significantly the presence
of getting the screen bounding rect on the CPU profile.
@mrobinson mrobinson force-pushed the mrobinson:clip-scroll-tree-opt-2 branch from 5d9498d to 1751ecc Dec 4, 2017
Some of the remaining methods in util.rs are duplicated in euclid
itself and others are now just simply unused.
@mrobinson
Copy link
Member Author

mrobinson commented Dec 4, 2017

Okay. I've rebased this branch as well as adding another, very simple, cleanup commit for util.rs.

@glennw
glennw approved these changes Dec 4, 2017
Copy link
Member

glennw left a comment

Nice!

@glennw
Copy link
Member

glennw commented Dec 4, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 4, 2017

📌 Commit 2901a69 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Dec 4, 2017

Testing commit 2901a69 with merge c76f272...

bors-servo added a commit that referenced this pull request Dec 4, 2017
Next set of ClipScrollTree optimizations

This seems to cut the prominence of ClipScrollTree updates in my profile a great deal.

<!-- 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/2156)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing c76f272 to master...

@bors-servo bors-servo merged commit 2901a69 into servo:master Dec 4, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 4 files, 1 discussion left (glennw)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:clip-scroll-tree-opt-2 branch Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.