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

Store picture indices directly in primitive instances. #3244

Merged
merged 4 commits into from Oct 31, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Oct 30, 2018

This removes the use of the BrushPrimitive for pictures, allowing
direct access to the picture index from the PrimitiveInstance.

Although there appears to be a lot of lines changed, it's mostly
just shuffling code around, there shouldn't be any functional
changes.

The motivation for this patch is to:

  • Remove use of primitive store for pictures (since these are
    being progressively removed and switched to primitive instances
    and interned primitive templates).
  • Most importantly, restructure the code such that we can add
    a primitive instance kind for primitives that only rely
    on the interned template, and not a primitive index. This will
    allow us to start interning primitives by type incrementally.

This change is Reviewable

gw3583 added some commits Oct 28, 2018

Refactor chase tracing to work with primitive instance IDs.
In the process of interning primitives, the PrimitiveIndex
will become less useful, eventually being removed since a
primitive instance will contain just a reference to an interned
primitive template.

Having the chase ID based on the PrimitiveIndex is complicating
some of that refactoring, so this patch switches it to create
a unique ID per primitive instance, which is then used by the
chase tracing code. The primitive ID only exists when the
tracing code is compiled in.
Introduce primitive clustering and use for some basic cases.
Primitive clusters are used to group primitives within a picture
into similar categories during scene building.

Initially, these are only used to group primitives by spatial node
and by backface flags. In future, these will include spatial
clustering, which will allow removal of much of the per-primitive
bounding rect and culling code during frame building.

In this patch each primitive belongs to one cluster only, but
when spatial clusters are introduced, a primitive will belong
to one or more cluster(s). Clusters are processed during the
initial picture traversal, allowing the picture bounding rect to
be found without touching all primitives.

There's a couple of goals of this work:
- Reduce the amount of comparison work we need to do to check
  if a cached picture surface is valid.
- Allow cached picture surfaces to be drawn in tiles, to
  handle cases where only a small portion of a picture
  contents has changed.

There's also a number of other small related changes:
- Store offscreen surface information outside the picture primitives.
  - Simplifies borrow check issues on picture traversal.
  - Eventually the surface structures will be retained between display lists, and hold cached surface details.

- Build minimal local clip rect for primitives during scene building.
  - Used to allow quick calculation of picture bounding rects.
  - Can take advantage of this to reduce work during frame building (not done yet).

- Picture bounding rect calculation is done during initial picture traversal pass.
  - Much faster than previously, as we only need to calculates bounds for each cluster.

 - Backface visibility is done once per cluster now, rather than per-primitive.
 - Intern the primitive local rect and clip rects for future use.
Store picture indices directly in primitive instances.
This removes the use of the BrushPrimitive for pictures, allowing
direct access to the picture index from the PrimitiveInstance.

Although there appears to be a lot of lines changed, it's mostly
just shuffling code around, there shouldn't be any functional
changes.

The motivation for this patch is to:
 - Remove use of primitive store for pictures (since these are
   being progressively removed and switched to primitive instances
   and interned primitive templates).
 - Most importantly, restructure the code such that we can add
   a primitive instance kind for primitives that only rely
   on the interned template, and not a primitive index. This will
   allow us to start interning primitives by type incrementally.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

This one is unfortunately difficult to review because (a) it includes #3238 and #3236, so looks bigger than it actually is and (b) has a large line diff count itself, due to re-shuffling and indentation changes.

r? anyone (it might make sense to wait until #3238 and #3236 are reviewed and merged, and then I'll rebase this PR on top of those to remove those commits).

@gw3583

This comment has been minimized.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

Some of the changes in this patch are a bit messy. It should become quite a bit tidier as we start to intern primitives, and split the prepare_prims pass completely from the picture traversal pass. But, if there's something in here that seems particularly untidy or problematic, let me know.

@kvark kvark self-requested a review Oct 30, 2018

@kvark

Amazing work! I got a few comments below, don't think there is anything blocking.

Reviewed 5 of 5 files at r1, 8 of 8 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @gw3583)


webrender/src/display_list_flattener.rs, line 914 at r1 (raw file):

                    container,
                );
                self.register_chase_primitive(

let's add _by_rect suffix to this method. It's just one way to set up the chase


webrender/src/display_list_flattener.rs, line 1231 at r3 (raw file):

            current_pic_index = blend_pic_index;

            cur_instance.kind = PrimitiveInstanceKind::Picture { pic_index: blend_pic_index };

these pieces look much better now 👍


webrender/src/picture.rs, line 46 at r2 (raw file):

    pub surface_index: SurfaceIndex,
    /// Used for backface visibility calculations.
    pub parent_spatial_node_index: SpatialNodeIndex,

we need to be careful with naming and semantics here. The backface visibility in general is determined by the "ancestor" spatial node, which happens to be the containing block in CSS terms. It's the immediate parent only for pictures that don't participate in 3D context. Is this one the ancestor, or an actual immediate parent?


webrender/src/picture.rs, line 65 at r2 (raw file):

impl SurfaceIndex {
    pub fn root() -> Self {

nit: make it a constant?


webrender/src/picture.rs, line 70 at r2 (raw file):

}

/// Information about an offscreen surface that. For now,

something is missing after "that." ?


webrender/src/picture.rs, line 272 at r2 (raw file):

}

/// Defines the grouping key for a cluster of

Is there a particular reasons for line wraps being so short here?


webrender/src/picture.rs, line 276 at r2 (raw file):

/// will also contain spatial grouping details.
#[derive(Hash, Eq, PartialEq, Copy, Clone)]
struct PrimitiveClusterKey {

I like the concept!


webrender/src/picture.rs, line 297 at r2 (raw file):

pub struct PrimitiveCluster {
    /// The positioning node for this cluster.
    spatial_node_index: SpatialNodeIndex,

should we just include the PrimitiveClusterKey here instead?


webrender/src/picture.rs, line 336 at r2 (raw file):

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

can we use std::ops::Range instead?


webrender/src/picture.rs, line 362 at r2 (raw file):

    /// List of primitives grouped into clusters.
    pub clusters: SmallVec<[PrimitiveCluster; 4]>,
    /// This maps from the cluster_range in a primitive

do the primitives need to know about their clusters? It would be great if there was less coupling, i.e. clusters knew about which primitive instances they contain but not the other way around


webrender/src/picture.rs, line 450 at r2 (raw file):

            let cluster_range = ClusterRange {
                first,
                last: first + 1,

the last is supposed to be "including" (judging by the name), yet it's assigned "first + 1" here despite the comment "since a primitive ever belongs to one cluster"


webrender/src/picture.rs, line 858 at r2 (raw file):

                // let wants_raster_root = xf.has_perspective_component() ||
                //                         local_scale.is_some();
                let establishes_raster_root = xf.has_perspective_component();

why aren't we having has_surface here any more?


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

#[cfg_attr(feature = "capture", derive(Serialize))]
#[cfg_attr(feature = "replay", derive(Deserialize))]
pub struct PrimitiveId(pub usize);

rename to PrimitiveDebugId?


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

                    VisibleFace::Front => {
                        if prim_instance.is_chased() {
                            #[cfg(debug_assertions)]

I don't think this is needed, given that is_chased is strictly false in non-debug


webrender/src/prim_store.rs, line 302 at r2 (raw file):

#[cfg_attr(feature = "replay", derive(Deserialize))]
pub struct PrimitiveSceneData {
    pub culling_rect: LayoutRect,

why is this a culling rect as opposed to clipping rect?

@gw3583

Reviewable status: 4 of 8 files reviewed, 14 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/display_list_flattener.rs, line 914 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's add _by_rect suffix to this method. It's just one way to set up the chase

Done.


webrender/src/display_list_flattener.rs, line 1231 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

these pieces look much better now 👍

Done.


webrender/src/picture.rs, line 46 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we need to be careful with naming and semantics here. The backface visibility in general is determined by the "ancestor" spatial node, which happens to be the containing block in CSS terms. It's the immediate parent only for pictures that don't participate in 3D context. Is this one the ancestor, or an actual immediate parent?

It's the immediate parent - it's used in the same way as the previous code (it matches on the 3d context state, and only uses the parent in certain cases).


webrender/src/picture.rs, line 65 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: make it a constant?

Done


webrender/src/picture.rs, line 70 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

something is missing after "that." ?

Done.


webrender/src/picture.rs, line 272 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Is there a particular reasons for line wraps being so short here?

Not really, fixed :)


webrender/src/picture.rs, line 276 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I like the concept!

Done.


webrender/src/picture.rs, line 297 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

should we just include the PrimitiveClusterKey here instead?

I wasn't quite sure if it would make sense to or not with future changes. I've left it as as for now, but it might make sense to in the future.


webrender/src/picture.rs, line 336 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we use std::ops::Range instead?

Done.


webrender/src/picture.rs, line 362 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

do the primitives need to know about their clusters? It would be great if there was less coupling, i.e. clusters knew about which primitive instances they contain but not the other way around

Yep, I agree. That does, however, complicate the current prepare_prims pass, since we'd need to traverse the clusters first, then correctly deal with prims duplicated in clusters and ensure the render order is preserved correctly. I do think that is what we'll end up with, but I thought I'd keep the changes to the prepare_prims loop minimal for now.


webrender/src/picture.rs, line 450 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

the last is supposed to be "including" (judging by the name), yet it's assigned "first + 1" here despite the comment "since a primitive ever belongs to one cluster"

Changed it to use ops::Range


webrender/src/picture.rs, line 858 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why aren't we having has_surface here any more?

has_surface was just a shortcut for actual_composite_mode.is_some(), but this code is a bit tidier now since it all only executes in the Some(actual_composite_mode) branch.


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

Previously, kvark (Dzmitry Malyshau) wrote…

rename to PrimitiveDebugId?

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

I don't think this is needed, given that is_chased is strictly false in non-debug

I'm not sure what this comment refers to - it might have been moved in later commits?


webrender/src/prim_store.rs, line 302 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why is this a culling rect as opposed to clipping rect?

Because it is clip_rect.intersection(prim_rect) which is the actual local rect that needs to be culled against.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

@kvark @emilio Thanks for the reviews! I think all comments are addressed now, and the try run looks good.

@@ -2180,7 +2180,7 @@ impl PrimitiveStore {
// an index buffer / set of primitive instances
// that we sort into render order.
let mut in_visible_cluster = false;
for ci in prim_instance.cluster_range.first .. prim_instance.cluster_range.last {
for ci in prim_instance.cluster_range.start .. prim_instance.cluster_range.end {

This comment has been minimized.

@kvark

kvark Oct 31, 2018

Member

nit: for ci in prim_instance.cluster_range.clone()

@kvark

kvark approved these changes Oct 31, 2018

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark)


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

Previously, gw3583 (Glenn Watson) wrote…

I'm not sure what this comment refers to - it might have been moved in later commits?

I reviewed the commits independently, and I think Reviewable reflects that when showing the comment. I'd expect it to have an option to jump on the actual code spot that was reviewed.

@kvark

This comment has been minimized.

Member

kvark commented Oct 31, 2018

Thanks, shipit!
@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 31, 2018

📌 Commit 3ec8aba has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 31, 2018

⌛️ Testing commit 3ec8aba with merge 62af01c...

bors-servo added a commit that referenced this pull request Oct 31, 2018

Auto merge of #3244 - gw3583:cluster-prim-instances-2, r=kvark
 Store picture indices directly in primitive instances.

This removes the use of the BrushPrimitive for pictures, allowing
direct access to the picture index from the PrimitiveInstance.

Although there appears to be a lot of lines changed, it's mostly
just shuffling code around, there shouldn't be any functional
changes.

The motivation for this patch is to:
 - Remove use of primitive store for pictures (since these are
   being progressively removed and switched to primitive instances
   and interned primitive templates).
 - Most importantly, restructure the code such that we can add
   a primitive instance kind for primitives that only rely
   on the interned template, and not a primitive index. This will
   allow us to start interning primitives by type incrementally.

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

This comment has been minimized.

Contributor

bors-servo commented Oct 31, 2018

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

@bors-servo bors-servo merged commit 3ec8aba into servo:master Oct 31, 2018

2 of 4 checks passed

Taskcluster (pull_request) TaskGroup: failure
Details
code-review/reviewable 2 discussions left (gw3583, kvark)
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