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

Convert ClipIds for Clips into indices in the ClipScrollTree #2427

Merged
merged 1 commit into from Feb 16, 2018

Conversation

@mrobinson
Copy link
Member

mrobinson commented Feb 15, 2018

During display list flattening, we convert ClipIds for the clipping node
into ClipChainIndex (as well as allocating space for them in the
ClipChain array in the ClipScrollTree). This allows us to avoid doing a
HashMap lookup for the ClipChain for every primitive run. It also moves
us a little closer to a universal representation of ClipChains (both
API-defined ClipChains and ones derived from the hierarchy of the
ClipScroll tree).

This causes the amount of time spend doing HashMap lookups to decrease
in the profile, though they are still done for the positioning node. A
second patch will reduce HashMap lookups for positioning nodes as well.


This change is Reviewable

@mrobinson mrobinson requested a review from kvark Feb 15, 2018
@mrobinson
Copy link
Member Author

mrobinson commented Feb 15, 2018

@mrobinson mrobinson requested a review from glennw Feb 15, 2018
@glennw
Copy link
Member

glennw commented Feb 16, 2018

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


webrender/src/frame_builder.rs, line 656 at r1 (raw file):

        self.reference_frame_stack.push(reference_frame_id);

        if let Some(ref parent_id) = parent_id {

match would probably be tidier here


webrender/src/hit_test.rs, line 298 at r1 (raw file):

    flags: HitTestFlags,
    node_cache: FastHashMap<ClipId, Option<LayerPoint>>,
    clip_chain_cache: Vec<Option<bool>>,

This could potentially be a bitvec or something, I think. But it's no big deal - I can't imagine it ever showing up in a profile.


webrender/src/render_task.rs, line 60 at r1 (raw file):

#[derive(Debug, Clone)]
pub struct ClipChain {

Not needed here - but it might make sense to move ClipChain to clip.rs as a follow up, since it's much more integral now?


Comments from Reviewable

@glennw
glennw approved these changes Feb 16, 2018
@glennw
Copy link
Member

glennw commented Feb 16, 2018

There's a couple of minor (optional) comments above, r=me either way. I'll leave it to you to merge when ready (not sure if you need to discuss with @nical any conflicts likely with the PR he's working on).

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

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

During display list flattening, we convert ClipIds for the clipping node
into ClipChainIndex (as well as allocating space for them in the
ClipChain array in the ClipScrollTree). This allows us to avoid doing a
HashMap lookup for the ClipChain for every primitive run. It also moves
us a little closer to a universal representation of ClipChains (both
API-defined ClipChains and ones derived from the hierarchy of the
ClipScroll tree).

This causes the amount of time spend doing HashMap lookups to decrease
in the profile, though they are still done for the positioning node. A
second patch will reduce HashMap lookups for positioning nodes as well.
@mrobinson mrobinson force-pushed the mrobinson:linear-clip-chain-indices branch from 94c1d68 to f11b8c7 Feb 16, 2018
@mrobinson
Copy link
Member Author

mrobinson commented Feb 16, 2018

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


webrender/src/frame_builder.rs, line 656 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

match would probably be tidier here

Oh, yeah. That's a bit better. I've made the change.


webrender/src/hit_test.rs, line 298 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This could potentially be a bitvec or something, I think. But it's no big deal - I can't imagine it ever showing up in a profile.

I'll save this one for later if that's okay.


webrender/src/render_task.rs, line 60 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Not needed here - but it might make sense to move ClipChain to clip.rs as a follow up, since it's much more integral now?

This makes sense to me as well and is relatively harmless, so I've gone ahead and moved the code now.


Comments from Reviewable

@mrobinson
Copy link
Member Author

mrobinson commented Feb 16, 2018

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

📌 Commit f11b8c7 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

Testing commit f11b8c7 with merge d2de084...

bors-servo added a commit that referenced this pull request Feb 16, 2018
Convert ClipIds for Clips into indices in the ClipScrollTree

During display list flattening, we convert ClipIds for the clipping node
into ClipChainIndex (as well as allocating space for them in the
ClipChain array in the ClipScrollTree). This allows us to avoid doing a
HashMap lookup for the ClipChain for every primitive run. It also moves
us a little closer to a universal representation of ClipChains (both
API-defined ClipChains and ones derived from the hierarchy of the
ClipScroll tree).

This causes the amount of time spend doing HashMap lookups to decrease
in the profile, though they are still done for the positioning node. A
second patch will reduce HashMap lookups for positioning nodes as well.

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

bors-servo commented Feb 16, 2018

💔 Test failed - status-travis

@mrobinson
Copy link
Member Author

mrobinson commented Feb 16, 2018

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Looks like the build stalled so I will retry.

@mrobinson
Copy link
Member Author

mrobinson commented Feb 16, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

Testing commit f11b8c7 with merge 41ae26b...

bors-servo added a commit that referenced this pull request Feb 16, 2018
Convert ClipIds for Clips into indices in the ClipScrollTree

During display list flattening, we convert ClipIds for the clipping node
into ClipChainIndex (as well as allocating space for them in the
ClipChain array in the ClipScrollTree). This allows us to avoid doing a
HashMap lookup for the ClipChain for every primitive run. It also moves
us a little closer to a universal representation of ClipChains (both
API-defined ClipChains and ones derived from the hierarchy of the
ClipScroll tree).

This causes the amount of time spend doing HashMap lookups to decrease
in the profile, though they are still done for the positioning node. A
second patch will reduce HashMap lookups for positioning nodes as well.

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

bors-servo commented Feb 16, 2018

💔 Test failed - status-travis

@mrobinson
Copy link
Member Author

mrobinson commented Feb 16, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

Testing commit f11b8c7 with merge ea86420...

bors-servo added a commit that referenced this pull request Feb 16, 2018
Convert ClipIds for Clips into indices in the ClipScrollTree

During display list flattening, we convert ClipIds for the clipping node
into ClipChainIndex (as well as allocating space for them in the
ClipChain array in the ClipScrollTree). This allows us to avoid doing a
HashMap lookup for the ClipChain for every primitive run. It also moves
us a little closer to a universal representation of ClipChains (both
API-defined ClipChains and ones derived from the hierarchy of the
ClipScroll tree).

This causes the amount of time spend doing HashMap lookups to decrease
in the profile, though they are still done for the positioning node. A
second patch will reduce HashMap lookups for positioning nodes as well.

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

bors-servo commented Feb 16, 2018

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

@bors-servo bors-servo merged commit f11b8c7 into servo:master Feb 16, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 9 files, 3 discussions left (glennw)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:linear-clip-chain-indices branch Feb 16, 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.