Simplify the ClipId -> CST id mapping code. #2925

gw3583 · 2018-07-23T02:10:15Z

There are some upcoming changes which will modify how and when
we store and represent clip nodes and chains. These changes are
necessary to allow WR to rasterize pictures in a different coordinate
space than just screen space.

This patch simplifies the mapping of ClipId to array indices in
the clip-scroll tree. This will make it simpler to make the
changes discussed above - specifically, we can unify how we handle
clip nodes with the per-primitive complex clips.

The code this removes was a performance optimization to avoid
hashmap lookups. I did some profiling on a number of real world
sites and also on some gecko layer stress tests, and didn't find
any noticeable regression from simplifying this code. However,
once the work above has been completed, we could re-investigate
how to optimize this mapping process, if it ever shows up in a
profile.

In one of the stress tests [1] for spatial and clip node creation,
the code is now slightly faster with this patch (28ms vs 29ms).

[1] https://mattwoodrow.github.io/dl-test/dl-test.html?count=50000&layer=inactive

This change is

gw3583 · 2018-07-23T02:10:30Z

Pending gecko try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c67919389ff58985dac7edc03227130126ca542

r? @mrobinson or @kvark

gw3583 · 2018-07-23T03:20:04Z

Gecko try looks good.

gw3583 · 2018-07-23T04:50:29Z

Pending try with the two follow up commits:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9d06e2bbe3539f630593cdd5f30ef7bd292993

gw3583 · 2018-07-23T06:02:27Z

Try still looks good 👍

kvark

LGTM, but needs @mrobinson to review as well if they are available

kvark · 2018-07-23T14:15:19Z

webrender/src/batch.rs

@@ -1772,16 +1772,16 @@ impl ClipBatcher {
    ) {
        let mut coordinate_system_id = coordinate_system_id;
        for work_item in clips.iter() {
+            let info = clip_store
+                .get_opt(&work_item.clip_sources)


nit: why get_opt if we follow with expect right away?

Because (for now) we only have a weak handle here, so the freelist API doesn't guarantee it's present, but we know that it should be due to usage here. I intend to refactor how we store clip sources as part of this work.

kvark · 2018-07-23T14:16:22Z

webrender/src/clip.rs

 }

 impl ClipSources {
-    pub fn new(clips: Vec<ClipSource>) -> Self {
+    pub fn new(
+        clips: Vec<ClipSource>,


we could use Iterator<Item = ClipSource> to avoid heap allocation on the client

It's not clear to me how this avoids an allocation in this case - could you expand on that?

This function doesn't need a vec, since it goes through the elements and constructs other stuff from them. But it's currently requiring the caller to have that vec. If this function gets IntoIterator instead, the caller may still provide a vec, or they can just construct a complex iterator (with chain(), zip(), etc) that is completely on the stack. IIRC, there is at least one place in code where this would be handy.

Ah, you're right - I misread that it took ownership of it, missing the collect. Although, having tried to make that change it seems to flow on to changing several other functions, so perhaps better to leave for now (since this PR doesn't actually change that code), unless I'm missing an obvious way to make this change locally?

It would be correct to change the calling functions as well, where possible, but at the same time Vec implements IntoIterator so those changes are not required. I'll be happy to follow up with this refactor ;)

kvark · 2018-07-23T14:39:25Z

webrender/src/display_list_flattener.rs

-        self.cached_clip_chain_index = Some((*id, index));
-        index
+    pub fn map_spatial_node(&mut self, id: ClipId, index: SpatialNodeIndex) {
+        debug_assert!(!self.spatial_node_map.contains_key(&id));


nit: could use the returned value from insert

kvark · 2018-07-23T14:40:47Z

webrender/src/hit_test.rs

@@ -34,6 +34,29 @@ pub struct HitTestClipNode {
    regions: Vec<HitTestRegion>,
 }

+impl HitTestClipNode {
+    fn new(node: &ClipNode, clip_store: &ClipStore) -> HitTestClipNode {


nit: -> Self

There are some upcoming changes which will modify how and when we store and represent clip nodes and chains. These changes are necessary to allow WR to rasterize pictures in a different coordinate space than just screen space. This patch simplifies the mapping of ClipId to array indices in the clip-scroll tree. This will make it simpler to make the changes discussed above - specifically, we can unify how we handle clip nodes with the per-primitive complex clips. The code this removes was a performance optimization to avoid hashmap lookups. I did some profiling on a number of real world sites and also on some gecko layer stress tests, and didn't find any noticeable regression from simplifying this code. However, once the work above has been completed, we could re-investigate how to optimize this mapping process, if it ever shows up in a profile. In one of the stress tests [1] for spatial and clip node creation, the code is now slightly faster with this patch (28ms vs 29ms). [1] https://mattwoodrow.github.io/dl-test/dl-test.html?count=50000&layer=inactive

kvark · 2018-07-24T01:36:51Z

webrender/src/display_list_flattener.rs

@@ -56,8 +56,8 @@ pub struct ClipIdToIndexMapper {

 impl ClipIdToIndexMapper {
    pub fn add_clip_chain(&mut self, id: ClipId, index: ClipChainIndex) {
-        debug_assert!(!self.clip_chain_map.contains_key(&id));
-        self.clip_chain_map.insert(id, index);
+        let _is_new_key = self.clip_chain_map.insert(id, index).is_none();


nit: let's not confuse the reader with is_none() here and instead move it inside the assert

mrobinson · 2018-07-24T05:48:48Z

If the LRU cache and non-hashing mapping aren't important to the code, then these changes simplify the code a lot. Looks good to me.

gw3583 · 2018-07-24T06:27:56Z

@mrobinson Thanks for taking a look! I think it's possible we will want the non-hashing optimization back in the future (especially for stress tests), but it doesn't seem to be providing a huge win right now (I wonder if we're either hashing less than we used to, or perhaps it's not such an issue now that we are using the Fx hasher instead of Fnv?).

mrobinson · 2018-07-24T06:33:50Z

@glennw It's an interesting question. It could be that this approach is still hashing quite a bit less than when we were not storing ClipScrollTree nodes in vectors and converting the ids to indexes.

kvark · 2018-07-24T14:55:07Z

@bors-servo r=kvark,mrobinson

bors-servo · 2018-07-24T14:55:08Z

📌 Commit 66ad1c4 has been approved by kvark,mrobinson

bors-servo · 2018-07-24T14:55:13Z

⌛ Testing commit 66ad1c4 with merge 3779039...

Simplify the ClipId -> CST id mapping code. There are some upcoming changes which will modify how and when we store and represent clip nodes and chains. These changes are necessary to allow WR to rasterize pictures in a different coordinate space than just screen space. This patch simplifies the mapping of ClipId to array indices in the clip-scroll tree. This will make it simpler to make the changes discussed above - specifically, we can unify how we handle clip nodes with the per-primitive complex clips. The code this removes was a performance optimization to avoid hashmap lookups. I did some profiling on a number of real world sites and also on some gecko layer stress tests, and didn't find any noticeable regression from simplifying this code. However, once the work above has been completed, we could re-investigate how to optimize this mapping process, if it ever shows up in a profile. In one of the stress tests [1] for spatial and clip node creation, the code is now slightly faster with this patch (28ms vs 29ms). [1] https://mattwoodrow.github.io/dl-test/dl-test.html?count=50000&layer=inactive  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2925)

bors-servo · 2018-07-24T16:15:59Z

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

@gw3583

Replace heap allocation of clip-related objects with iteration This PR makes scene building faster by doing less heap allocations. r? @gankro or @gw3583 cc @gw3583 - this is a follow-up to #2925 that I figured I'd forget about if I don't do it now ;)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2943)

kvark approved these changes Jul 23, 2018

View reviewed changes

gw3583 added 3 commits July 24, 2018 09:12

Move SpatialNodeIndex into ClipSources.

86bd037

Remove SpatialNodeIndex from ClipNode.

8593ac0

kvark reviewed Jul 24, 2018

View reviewed changes

Address review comments.

66ad1c4

gw3583 force-pushed the map2 branch from 87bddf9 to 66ad1c4 Compare July 24, 2018 03:03

kvark approved these changes Jul 24, 2018

View reviewed changes

bors-servo merged commit 66ad1c4 into servo:master Jul 24, 2018

kvark mentioned this pull request Jul 31, 2018

Replace heap allocation of clip-related objects with iteration #2943

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the ClipId -> CST id mapping code. #2925

Simplify the ClipId -> CST id mapping code. #2925

gw3583 commented Jul 23, 2018 •

edited by larsbergstrom

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

kvark left a comment

kvark Jul 23, 2018

gw3583 Jul 23, 2018

kvark Jul 23, 2018

gw3583 Jul 23, 2018

kvark Jul 24, 2018

gw3583 Jul 24, 2018

kvark Jul 24, 2018

kvark Jul 23, 2018

gw3583 Jul 23, 2018

kvark Jul 23, 2018

gw3583 Jul 23, 2018

kvark Jul 24, 2018

gw3583 Jul 24, 2018

mrobinson commented Jul 24, 2018

gw3583 commented Jul 24, 2018

mrobinson commented Jul 24, 2018 •

edited

kvark commented Jul 24, 2018

bors-servo commented Jul 24, 2018

bors-servo commented Jul 24, 2018

bors-servo commented Jul 24, 2018

Simplify the ClipId -> CST id mapping code. #2925

Simplify the ClipId -> CST id mapping code. #2925

Conversation

gw3583 commented Jul 23, 2018 • edited by larsbergstrom

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

gw3583 commented Jul 23, 2018

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson commented Jul 24, 2018

gw3583 commented Jul 24, 2018

mrobinson commented Jul 24, 2018 • edited

kvark commented Jul 24, 2018

bors-servo commented Jul 24, 2018

bors-servo commented Jul 24, 2018

bors-servo commented Jul 24, 2018

gw3583 commented Jul 23, 2018 •

edited by larsbergstrom

mrobinson commented Jul 24, 2018 •

edited