Embed PrimitiveMetadata inside Primitive struct. #2965

gw3583 · 2018-08-10T08:31:55Z

This is another small step to moving primitives to be stored inside Picture structs, which will simplify some of the picture caching work.

While making that change, I also tidied up some of the prim_store code by moving several of the methods into the Primitive struct. I've done each of these as a separate commit to make it easier to review.

There's no functional changes here - just moving code around to make the picture caching work easier. There's also quite a few array indexing operations removed, which might be a small performance win.

This change is

gw3583 · 2018-08-10T08:33:01Z

r? @kvark

This looks like a big PR, but most of it is just moving methods around as a result of the first commit. I tried to separate each method change into a single commit to make it easier to review.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee64481c801ff2df8b85fa01e915a598fac9e13

There's a few orange items, but I think they are all unrelated - probably best to take a quick look to make sure though.

kvark

There is a ton of things moved around, making it difficult to review. But overall I believe the changes are straightforward and improving our code.

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gw3583)

webrender/src/batch.rs, line 1573 at r1 (raw file):

}

trait AlphaBatchHelpers {

glad to see this going away :)

webrender/src/picture.rs, line 225 at r1 (raw file):

    }

    pub fn update_local_rect(

re-assigning the runs here goes across the name of the method, need to think of a better name

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

pub enum PrimitiveDetails {
    Brush(BrushPrimitive),
    TextRun(TextRunPrimitiveCpu),

Would be nice to make this more consistent: either BrushPrimitiveCpu or TextRunPrimitive

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

pub struct Primitive {
    pub metadata: PrimitiveMetadata,
    pub details: PrimitiveDetails,

I'm a tiny bit worried that putting those together (as in - going with array of structures versus structure of arrays) would hurt our data locality.

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

    pub fn as_pic(&self) -> &PicturePrimitive {
        match self.details {
            PrimitiveDetails::Brush(ref brush) => {

nit: could do in a single match: PrimitiveDerails::Brush(BrushPrimitive { kind: BrushKind::Picture(), .. })

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

        // the collapsed primitive will be drawn directly into the
        // parent picture.
        self.get_pic_mut(pic_prim_index).composite_mode = None;

Sounds like we can move it into the match arm and call an prim itself. Perhaps, we'll not need the get_pic_mut wrapper at all then?

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

}

fn write_brush_segment_description(

eh, it's hard to review this function since it's moved

webrender/src/prim_store.rs, line 2348 at r5 (raw file):

    fn prepare_prim_for_render_inner(
        &mut self,
        prim_index: PrimitiveIndex,

a bit strange now that this function requires the primitive index

webrender/src/prim_store.rs, line 2377 at r5 (raw file):

            }
            PrimitiveDetails::Brush(ref mut brush) => {

nit: extra line?

webrender/src/prim_store.rs, line 2388 at r5 (raw file):

                        ref mut visible_tiles,
                        ..
                    } => {

we need to refactor this match arm in the future, it has too much logic

webrender/src/prim_store.rs, line 2722 at r7 (raw file):

        if let BrushKind::Border { ref mut source, .. } = brush.kind {
            if let BorderSource::Border {

nit: can combine in a single if let to reduce indentation

kvark · 2018-08-10T17:11:51Z

@gw3583 feel free to r=me if having no desire to address the nits :)

bors-servo · 2018-08-10T19:10:58Z

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


        Embed PrimitiveMetadata inside Primitive struct.

This is another small step to moving primitives to be stored inside Picture structs, which will simplify some of the picture caching work.


        Move write_brush_segment_description to a free function.


        Move update_clip_task_for_brush to method of Primitive.


        Move reset_clip_mask to be method of Primitive.


        Move prepare_prim_for_render_inner to method of Primitive


        Move update_clip_task to method of Primitive.


        Move build_prim_segments_if_needed to Primitive


        Remove a borrowck hack


        Remove another borrowck hack.


        Tidy up prepare_prim_for_render a little


        Address review comments.

gw3583

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

webrender/src/batch.rs, line 1573 at r1 (raw file):