Port normal (css) style borders to be interned primitives. #3342

gw3583 · 2018-11-23T00:22:07Z

This is mostly a straightforward conversion of normal borders
to make use of primitive interning.

It also adds support during batching and clip task creation for
interned primitives that make use of segments. This is needed
for porting the rest of the primitives, and should make the
conversion of the other primitive types simple.

This change is

gw3583 · 2018-11-23T00:22:40Z

r? anyone

Try run looks good (sans seemingly unrelated intermittents):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=858b26bf8844094a5870f8c172d88249273ff90f

gw3583 · 2018-11-23T00:52:51Z

Added a follow up commit that also ports image borders to be interned.

New pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02be9f170cef1af94e62ffdb8fce438d354836fa

gw3583 · 2018-11-23T01:01:23Z

Some of these changes are quite messy, with duplicated code in places. This could definitely do with a tidy / clean up pass once all the primitive types are ported and we remove the BrushPrimitive type.

bors-servo · 2018-11-23T02:20:40Z

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

gw3583 · 2018-11-23T02:36:59Z

Squashed and rebased on the picture caching commit.

Try run looks good, I think. There are a couple of crash test failures (with known intermittents), and a border failure in R1 on OSX. The border failure seems to be text related, I think it's probably due to one of the other recent commits in master?

kvark

I don't think there are any blockers

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @gw3583)

webrender/src/border.rs, line 107 at r1 (raw file):

#[cfg_attr(feature = "replay", derive(Deserialize))]
#[derive(Debug, Clone, Hash, Eq, PartialEq)]
pub struct NormalBorderAu {

if it matches NormalBorder outside of Au vs f32, can we just have NormalBoder<Au> and NormalBorder<f32>?

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

                                let descriptor = BrushSegmentDescriptor {
                                    segments: SmallVec::from_vec(segments),

we might as well just keep a Vec here - I'll fix up some allocations in follow-ups (if we proceed like this)

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

        font: FontInstance,
        offset: LayoutVector2DAu,
        glyphs: Vec<GlyphInstance>,

Now that I'm looking at it, why does primitive key kind have a list of glyph instances? Esp since PrimitiveContainer has that list as well

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

        request: ImageRequest,
        widths: LayoutSideOffsetsAu,
        width: i32,

any reason this isn't some TypedSize2D<i32, ..>?

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

    },
    NormalBorder {
        template: Box<NormalBorderTemplate>,

does this need to be a box?

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

/// Information about how to cache a border segment,
/// along with the current render task cache entry.
#[cfg_attr(feature = "capture", derive(Serialize))]

I don't know if this is required here, but I'm happy to see more stuff being serialized. Just gives us a better chance of looking at the captures later down the road ;)

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

    },
    ImageBorder {
        request: ImageRequest,

if we put those fields into a (generic) structure, we can re-use it in PrimitiveKeyKind

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

    pub glyph_keys: GlyphKeyStorage,

    pub border_cache_handles: BorderHandleStorage,

let's keep a good tradition of documenting things :)

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

                    handles.push(frame_state.resource_cache.request_render_task(
                        cache_key.clone(),

redundant clone() ?

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

                    .scratch
                    .border_cache_handles
                    .extend(handles);

looks like we could have a similar scheme here as we do for GPU cache - some sort of accessor temporary that allows writing directly to the storage, instead of collecting into a SmallVec first like we do here

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

                PrimitiveInstanceKind::ImageBorder { .. },
                PrimitiveTemplateKind::ImageBorder { .. }
            ) => {

nothing to do here?

webrender/src/storage.rs, line 81 at r1 (raw file):

    }

    pub fn clear(&mut self) {

we need to add an Epoch to the Index<T> under cfg(debug_assertions), bumping it when the storage gets cleared/recycled
this is fine to do later, I'll make a note for myself ;)

webrender_api/src/units.rs, line 227 at r1 (raw file):

}

impl AuHelpers<LayoutSideOffsetsAu> for LayoutSideOffsets {

I noticed that there is a few places where we are manually converting some XxxSideoffsets between Au and what not.
Can we make this implementation generic over U (being the pixel type), so that the other places can avoid re-implementing it (should be easy to find by "from_32_px")?


        Port borders to be interned primitives.

This is mostly a straightforward conversion of borders to make use of primitive interning. It also adds support during batching and clip task creation for interned primitives that make use of segments. This is needed for porting the rest of the primitives, and should make the conversion of the other primitive types simple.

gw3583

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kvark)

webrender/src/border.rs, line 107 at r1 (raw file):