Separate brush segment descriptors from clip mask instances. #3289

gw3583 · 2018-11-09T02:31:04Z

As part of interning primitives, we can move the segmenting
process to occur when a primitive is newly interned (which is
a significant optimization opportunity). We can do this since
segmentation only occurs on clip nodes with the same spatial
node as the primitive, thus the segmentation cannot be changed
due to scrolling or transform animation.

However, the presence of a clip mask per segment may still
change each frame, due to clips from other positioning nodes.

To handle this, we need to split the per-frame clip mask instance
information from the brush segment descriptors.

This is also a necessary step to allow borders to be interned,
which rely on pre-generated segment descriptors.

At the same time, make the size of a RenderTaskId smaller in
release builds, and tidy up FrameId::invalid().

This change is

gw3583 · 2018-11-09T02:31:11Z

r? @kvark or @nical

gw3583 · 2018-11-09T02:31:43Z

This also seems to give a small but noticeable performance win on some of the talos tests.

Try run is not complete but looks good so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7481e1ced995bf0119a53472481a487ab28044

gw3583 · 2018-11-09T02:34:21Z

Mentioned in the comments but repeating here too - the work here to maintain per-instance data between prepare_prims and batching should be temporary. In the near-ish future we should be able to unify those passes, and avoid storing any of this data.

gw3583 · 2018-11-09T03:29:36Z

Try run looks good.

kvark

Looks good, I have a few non-blocking concerns.

we can move the segmenting process to occur when a primitive is newly interned

where is this done?

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

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

        // If a got a valid (or OPAQUE) clip task address, add the segment.
        if let Some(clip_task_address) = clip_task_address {

nit: just return early if it's None?

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

}

impl PrimitiveStore {

does this have to be in batch module as opposed to the prim_store as all the rest of PrimitiveStore logic?

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

        }

        self.prim_store.begin_frame();

nit: rename to something more specific, e.g. reset_clip_instances?

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

                if !clip_chain.needs_mask ||
                   (!self.may_need_clip_mask && !clip_chain.has_non_local_clips) {
                    clip_mask_instances.push(ClipMaskKind::None);

does this function always end up just pushing one ClipMaskKind? could it instead just return it?

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

    #[cfg(debug_assertions)]
    pub frame_id: FrameId,

does it need to be pub?

bors-servo · 2018-11-09T18:54:31Z

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

As part of interning primitives, we can move the segmenting process to occur when a primitive is newly interned (which is a significant optimization opportunity). We can do this since segmentation only occurs on clip nodes with the same spatial node as the primitive, thus the segmentation cannot be changed due to scrolling or transform animation. However, the presence of a clip mask per segment may still change each frame, due to clips from other positioning nodes. To handle this, we need to split the per-frame clip mask instance information from the brush segment descriptors. This is also a necessary step to allow borders to be interned, which rely on pre-generated segment descriptors. At the same time, make the size of a RenderTaskId smaller in release builds, and tidy up FrameId::invalid().

gw3583

Reviewable status: 2 of 10 files reviewed, 3 unresolved discussions (waiting on @kvark)

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