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

Assign all ClipIds during display list construction #2402

Merged
merged 1 commit into from Feb 9, 2018

Conversation

@mrobinson
Copy link
Member

mrobinson commented Feb 9, 2018

This change makes it so that all ClipIds are assigned during display
list construction. This will allow us to count the number of ClipIds in
each display list during construction, making it possible to more easily
map them onto simple array indices. The final goal of this work is to
eliminate the performance cost of hashing when flattening and rendering
display lists.


This change is Reviewable

@mrobinson mrobinson requested review from kvark and glennw Feb 9, 2018
@kvark
Copy link
Member

kvark commented Feb 9, 2018

:lgtm:


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


webrender_api/src/display_item.rs, line 763 at r1 (raw file):

impl ClipId {
    pub fn root_scroll_node(pipeline_id: PipelineId) -> ClipId {
        ClipId::Clip(1, pipeline_id)

can we define it as a constant?


webrender_api/src/display_list.rs, line 812 at r1 (raw file):

        let start_time = precise_time_ns();

        // We start at w here, because the root reference is always 0 and the root scroll

start at w?


webrender_api/src/display_list.rs, line 1316 at r1 (raw file):

        filters: Vec<FilterOp>,
    ) {
        let reference_frame_id = if transform.is_some() || perspective.is_some() {

to clarify, we aren't checking for axis alignment here because an animated transform always spawns a reference frame, right?


Comments from Reviewable

@mrobinson mrobinson force-pushed the mrobinson:assign-clipids-eagerly branch from a676978 to 6870c17 Feb 9, 2018
@mrobinson
Copy link
Member Author

mrobinson commented Feb 9, 2018

Review status: 4 of 6 files reviewed at latest revision, 3 unresolved discussions.


webrender_api/src/display_item.rs, line 763 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we define it as a constant?

Done.


webrender_api/src/display_list.rs, line 812 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

start at w?

Heh. This was supposed to be a '2'. Fixed. :)


webrender_api/src/display_list.rs, line 1316 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

to clarify, we aren't checking for axis alignment here because an animated transform always spawns a reference frame, right?

That's correct. The spec dictates that even an identity transform creates a stacking context.


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Feb 9, 2018

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


webrender_api/src/display_list.rs, line 814 at r2 (raw file):

        // We start at 2 here, because the root reference is always 0 and the root scroll
        // node is always 1.
        const FIRST_CLIP_ID: u64 = 2;

can we move it up next to the rest of the constants?


Comments from Reviewable

This change makes it so that all ClipIds are assigned during display
list construction. This will allow us to count the number of ClipIds in
each display list during construction, making it possible to more easily
map them onto simple array indices. The final goal of this work is to
eliminate the performance cost of hashing when flattening and rendering
display lists.
@mrobinson mrobinson force-pushed the mrobinson:assign-clipids-eagerly branch from 6870c17 to 1e030a2 Feb 9, 2018
@mrobinson
Copy link
Member Author

mrobinson commented Feb 9, 2018

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.


webrender_api/src/display_list.rs, line 814 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we move it up next to the rest of the constants?

Done.


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Feb 9, 2018

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Feb 9, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2018

📌 Commit 1e030a2 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2018

Testing commit 1e030a2 with merge 7184d6b...

bors-servo added a commit that referenced this pull request Feb 9, 2018
Assign all ClipIds during display list construction

This change makes it so that all ClipIds are assigned during display
list construction. This will allow us to count the number of ClipIds in
each display list during construction, making it possible to more easily
map them onto simple array indices. The final goal of this work is to
eliminate the performance cost of hashing when flattening and rendering
display lists.

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

bors-servo commented Feb 9, 2018

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

@bors-servo bors-servo merged commit 1e030a2 into servo:master Feb 9, 2018
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 6 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Feb 9, 2018
6 of 7 tasks complete
@mrobinson mrobinson deleted the mrobinson:assign-clipids-eagerly branch Feb 9, 2018
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.