Store ClipScrollNodes in a vector #2449

mrobinson · 2018-02-21T14:42:34Z

Instead of storing ClipScrollNodes in a HashMap, store them in a vector.
This removes the CPU cost of accessing any particular ClipScrollNode. In
its place is a somewhat clunky system of translating a ClipId into a
ClipScrollIndex. These are allocated based on the total number of
ClipIds per display list, so the translation should be fairly cheap. A
per-pipeline offset and cache is stored, so the number of HashMap
lookups for display list flattening should be the same order as the
number of pipelines.

The big trade off here is that when descending into iframes, we need to
allocate space in the ClipScrollNode array ahead of time. If these
memory allocations turn out to be a problem, we can modify the code to
try to mutate existing nodes or to use a more complicated system of sub
vectors or slices.

This change is

mrobinson · 2018-02-21T14:43:16Z

Sorry for the size of this PR. It's difficult to make such a deep change without changing a lot of code.

Here is the Gecko try job for this change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8950bf1f433b3fd3b71b5cb526d9cb71fea98770&selectedJob=163434876

mrobinson · 2018-02-21T14:51:44Z

@staktrace Heads up about the Gecko changes. I had to modify gfx/webrender_bindings/webrender_ffi_generated.h manually because it seems that cbindgen has trouble with the current WebRender source tree.

staktrace · 2018-02-21T17:19:54Z

I feel like the gecko-side changes should either assert that u64 and usize have the same number of bits (which might not actually be true on all platforms) or better, we should replace uint64_t with size_t wherever we're keeping WR clipids in C++. This is probably hard to do at the moment but will be a lot simpler if I land my changes for using ClipChains (but that's blocked on #2388).

kvark · 2018-02-21T20:14:58Z

Looks like a decent optimization, at the cost of increased re-direction complexity of the code.

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

webrender/src/clip_scroll_tree.rs, line 135 at r1 (raw file):

        // TODO(mrobinson): We should eventually make this impossible to misuse.
        debug_assert!(self.nodes.len() >= 1);
        ClipScrollNodeIndex(1)

didn't we have constants defined for those special indices somewhere?

webrender/src/clip_scroll_tree.rs, line 466 at r1 (raw file):

            if let Some((offset, clamping)) = self.pending_scroll_offsets.remove(&external_id) {

why is this removed?

webrender/src/clip_scroll_tree.rs, line 505 at r1 (raw file):

        }

        if index.0 == self.nodes.len() {

I'd refactor that code into:

if let Some(ref mut empty_node) = self.nodes.get_mut(index.0) {
    *empty_node = node;
    return
}
self.nodes.reserve_exact(index.0 + 1 - self.nodes.len());
self.nodes.extend((self.nodes.len() .. index.0).map(|_| ClipScrollNode::empty());
self.nodes.push(node);

webrender/src/clip_scroll_tree.rs, line 511 at r1 (raw file):

        if index.0 > self.nodes.len() {
            self.nodes.reserve_exact(index.0);

this is incorrect usage of reserve_exact. The parameter is meant to be the additional count, not total count

webrender/src/frame.rs, line 49 at r1 (raw file):

    cached_index: Option<(ClipId, ClipChainIndex)>,
    pipeline_offsets: FastHashMap<PipelineId, usize>,
    cached_pipeline_offset: Option<(PipelineId, usize)>,

need to comment cleanly the difference between cached_pipeline_offset and pipeline_offsets

webrender/src/frame.rs, line 54 at r1 (raw file):

impl ClipIdToIndexMapper {
    fn new() -> ClipIdToIndexMapper {

nit: Self ftw!

webrender/src/frame.rs, line 58 at r1 (raw file):

            clip_chain_map: FastHashMap::default(),
            cached_index: None,
            pipeline_offsets: FastHashMap::default(),

the whole implementation can probably be replaced by derive(Default)

webrender/src/frame.rs, line 60 at r1 (raw file):

            pipeline_offsets: FastHashMap::default(),
            cached_pipeline_offset: None,
            next_available_offset: 0,

what's the lifetime of those fields? I don't see them being reset

webrender/src/frame.rs, line 83 at r1 (raw file):

    }

    pub fn get_clip_chain_index_and_cache_result(&mut self, id: &ClipId) -> ClipChainIndex {

nit: maybe cache_clip_chain_index? to indicate that it's mutating

webrender/src/frame.rs, line 109 at r1 (raw file):

        let (index, pipeline_id) = match id {
            ClipId::Clip(index, pipeline_id) => (index, pipeline_id),
            ClipId::ClipChain(_) => unreachable!("Tried to use ClipChain as scroll node."),

not sure if this is truly unreachable. Sounds more like a panic to me

webrender/src/hit_test.rs, line 130 at r1 (raw file):

            let index = ClipScrollNodeIndex(index);
            if !self.pipeline_root_nodes.contains_key(&node.pipeline_id) {
                self.pipeline_root_nodes.insert(node.pipeline_id, index);

nit: use Entry API

webrender_api/src/display_item.rs, line 771 at r1 (raw file):

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
pub enum ClipId {

This is significantly less used now. We should take another look at the type and see if it's still needed, or maybe rename it to something more verbose

webrender_api/src/display_list.rs, line 518 at r1 (raw file):

                    },
                    ClipChain(specific_item, clip_chain_ids) => {
                        DisplayListBuilder::push_iter_impl(&mut temp, clip_chain_ids);

clip chain doesn't count for total clips?

Comments from Reviewable

mrobinson · 2018-02-22T08:52:49Z

Review status: all files reviewed at latest revision, 13 unresolved discussions.

webrender/src/clip_scroll_tree.rs, line 135 at r1 (raw file):