Bug 1520384 - Fix an invalidation bug and improve display list correlation. r=kvark
authorGlenn Watson <github@intuitionlibrary.com>
Wed, 16 Jan 2019 04:23:29 +0000
changeset 514038 8cef8c45741cb04fc769f56d3d6bd5a42a918685
parent 514037 79c6f7924680f978c9205df25653a16cec378e21
child 514039 53a5c9148120d2716d8f668f2a81edf71016d0f4
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskvark
bugs1520384
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1520384 - Fix an invalidation bug and improve display list correlation. r=kvark Differential Revision: https://phabricator.services.mozilla.com/D16648
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/renderer.rs
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -31,16 +31,17 @@ use render_backend::FrameResources;
 use render_task::{ClearMode, RenderTask, RenderTaskCacheEntryHandle, TileBlit};
 use render_task::{RenderTaskCacheKey, RenderTaskCacheKeyKind, RenderTaskId, RenderTaskLocation};
 use resource_cache::ResourceCache;
 use scene::{FilterOpHelpers, SceneProperties};
 use scene_builder::DocumentResources;
 use smallvec::SmallVec;
 use surface::{SurfaceDescriptor};
 use std::{mem, u16};
+use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
 use texture_cache::{Eviction, TextureCacheHandle};
 use tiling::RenderTargetKind;
 use util::{ComparableVec, TransformedRectKind, MatrixHelpers, MaxRect};
 
 /*
  A picture represents a dynamically rendered image. It consists of:
 
  * A number of primitives that are drawn onto the picture.
@@ -73,16 +74,17 @@ impl RetainedTiles {
         RetainedTiles {
             tiles: Vec::new(),
             ref_prims: FastHashMap::default(),
         }
     }
 
     /// Merge items from one retained tiles into another.
     pub fn merge(&mut self, other: RetainedTiles) {
+        assert!(self.tiles.is_empty() || other.tiles.is_empty());
         self.tiles.extend(other.tiles);
         self.ref_prims.extend(other.ref_prims);
     }
 }
 
 /// Unit for tile coordinates.
 #[derive(Hash, Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)]
 pub struct TileCoordinate;
@@ -105,22 +107,23 @@ const FRAMES_BEFORE_CACHING: usize = 2;
 //           profiling / telemetry to see when it makes sense
 //           to cache a picture.
 const MAX_CACHE_SIZE: f32 = 2048.0;
 /// The maximum size per axis of a surface,
 ///  in WorldPixel coordinates.
 const MAX_SURFACE_SIZE: f32 = 4096.0;
 
 
-/// The number of primitives to search for, trying to correlate
-/// the offset between one display list and another.
-const MAX_PRIMS_TO_CORRELATE: usize = 64;
-/// The minmum number of primitives we need to correlate in
-/// order to consider it a success.
-const MIN_PRIMS_TO_CORRELATE: usize = MAX_PRIMS_TO_CORRELATE / 4;
+/// The maximum number of primitives to look for in a display
+/// list, trying to find unique primitives.
+const MAX_PRIMS_TO_SEARCH: usize = 128;
+
+/// Used to get unique tile IDs, even when the tile cache is
+/// destroyed between display lists / scenes.
+static NEXT_TILE_ID: AtomicUsize = ATOMIC_USIZE_INIT;
 
 /// Information about the state of an opacity binding.
 #[derive(Debug)]
 pub struct OpacityBindingInfo {
     /// The current value retrieved from dynamic scene properties.
     value: f32,
     /// True if it was changed (or is new) since the last frame build.
     changed: bool,
@@ -208,27 +211,33 @@ impl Tile {
 
     /// Clear the dependencies for a tile.
     fn clear(&mut self) {
         self.transforms.clear();
         self.descriptor.clear();
         self.potential_clips.clear();
     }
 
-    /// Update state related to whether a tile has the same
-    /// content and is valid to use.
-    fn update_validity(&mut self, tile_bounding_rect: &WorldRect) {
+    /// Invalidate a tile based on change in content. This
+    /// muct be called even if the tile is not currently
+    /// visible on screen. We might be able to improve this
+    /// later by changing how ComparableVec is used.
+    fn update_content_validity(&mut self) {
         // Check if the contents of the primitives, clips, and
         // other dependencies are the same.
         self.is_same_content &= self.descriptor.is_same_content();
-
+        self.is_valid &= self.is_same_content;
+    }
+
+    /// Update state related to whether a tile has a valid rect that
+    /// covers the required visible part of the tile.
+    fn update_rect_validity(&mut self, tile_bounding_rect: &WorldRect) {
         // The tile is only valid if:
         // - The content is the same *and*
         // - The valid part of the tile includes the needed part.
-        self.is_valid &= self.is_same_content;
         self.is_valid &= self.valid_rect.contains_rect(tile_bounding_rect);
 
         // Update count of how many times this tile has had the same content.
         if !self.is_same_content {
             self.same_frames = 0;
         }
         self.same_frames += 1;
     }
@@ -357,78 +366,122 @@ pub struct TileCache {
     /// A list of blits from the framebuffer to be applied during this frame.
     pub pending_blits: Vec<TileBlit>,
     /// The current world bounding rect of this tile cache. This is used
     /// to derive a local clip rect, such that we don't obscure in the
     /// z-buffer any items placed earlier in the render order (such as
     /// scroll bars in gecko, when the content overflows under the
     /// scroll bar).
     world_bounding_rect: WorldRect,
-    /// Counter for the next id to assign for a new tile.
-    next_id: usize,
     /// List of reference primitive information used for
     /// correlating the position between display lists.
-    reference_prims: Vec<ReferencePrimitive>,
+    reference_prims: ReferencePrimitiveList,
     /// The root clip chain for this tile cache.
     root_clip_chain_id: ClipChainId,
 }
 
 /// Stores information about a primitive in the cache that we will
 /// try to use to correlate positions between display lists.
+#[derive(Clone)]
 struct ReferencePrimitive {
     uid: ItemUid,
     local_pos: LayoutPoint,
     spatial_node_index: SpatialNodeIndex,
+    ref_count: usize,
+}
+
+/// A list of primitive with uids that only exist once in a display
+/// list. Used to obtain reference points to correlate the offset
+/// between two similar display lists.
+struct ReferencePrimitiveList {
+    ref_prims: Vec<ReferencePrimitive>,
+}
+
+impl ReferencePrimitiveList {
+    fn new(
+        prim_instances: &[PrimitiveInstance],
+        pictures: &[PicturePrimitive],
+    ) -> Self {
+        let mut map = FastHashMap::default();
+        let mut search_count = 0;
+
+        // Collect a set of primitives that we can
+        // potentially use for correlation.
+        collect_ref_prims(
+            prim_instances,
+            pictures,
+            &mut map,
+            &mut search_count,
+        );
+
+        // Select only primitives where the uid is unique
+        // in the display list, giving the best chance
+        // of finding correct correlations.
+        let ref_prims = map.values().filter(|prim| {
+            prim.ref_count == 1
+        }).cloned().collect();
+
+        ReferencePrimitiveList {
+            ref_prims,
+        }
+    }
 }
 
 /// Collect a sample of primitives from the prim list that can
 /// be used to correlate positions.
-// TODO(gw): Investigate best how to select which primitives to select.
 fn collect_ref_prims(
     prim_instances: &[PrimitiveInstance],
-    ref_prims: &mut Vec<ReferencePrimitive>,
     pictures: &[PicturePrimitive],
+    map: &mut FastHashMap<ItemUid, ReferencePrimitive>,
+    search_count: &mut usize,
 ) {
     for prim_instance in prim_instances {
-        if ref_prims.len() >= MAX_PRIMS_TO_CORRELATE {
+        if *search_count > MAX_PRIMS_TO_SEARCH {
             return;
         }
 
         match prim_instance.kind {
             PrimitiveInstanceKind::Picture { pic_index, .. } => {
                 collect_ref_prims(
                     &pictures[pic_index.0].prim_list.prim_instances,
-                    ref_prims,
                     pictures,
+                    map,
+                    search_count,
                 );
             }
             _ => {
-                ref_prims.push(ReferencePrimitive {
-                    uid: prim_instance.uid(),
-                    local_pos: prim_instance.prim_origin,
-                    spatial_node_index: prim_instance.spatial_node_index,
+                let uid = prim_instance.uid();
+
+                let entry = map.entry(uid).or_insert_with(|| {
+                    ReferencePrimitive {
+                        uid,
+                        local_pos: prim_instance.prim_origin,
+                        spatial_node_index: prim_instance.spatial_node_index,
+                        ref_count: 0,
+                    }
                 });
+                entry.ref_count += 1;
+
+                *search_count = *search_count + 1;
             }
         }
     }
 }
 
 impl TileCache {
     pub fn new(
         spatial_node_index: SpatialNodeIndex,
         prim_instances: &[PrimitiveInstance],
         root_clip_chain_id: ClipChainId,
         pictures: &[PicturePrimitive],
     ) -> Self {
         // Build the list of reference primitives
         // for this picture cache.
-        let mut reference_prims = Vec::with_capacity(MAX_PRIMS_TO_CORRELATE);
-        collect_ref_prims(
+        let reference_prims = ReferencePrimitiveList::new(
             prim_instances,
-            &mut reference_prims,
             pictures,
         );
 
         TileCache {
             spatial_node_index,
             tiles: Vec::new(),
             map_local_to_world: SpaceMapper::new(
                 ROOT_SPATIAL_NODE_INDEX,
@@ -439,28 +492,21 @@ impl TileCache {
             dirty_region: None,
             needs_update: true,
             world_origin: WorldPoint::zero(),
             world_tile_size: WorldSize::zero(),
             tile_count: TileSize::zero(),
             scroll_offset: None,
             pending_blits: Vec::new(),
             world_bounding_rect: WorldRect::zero(),
-            next_id: 0,
             reference_prims,
             root_clip_chain_id,
         }
     }
 
-    fn next_id(&mut self) -> TileId {
-        let id = TileId(self.next_id);
-        self.next_id += 1;
-        id
-    }
-
     /// Get the tile coordinates for a given rectangle.
     fn get_tile_coords_for_rect(
         &self,
         rect: &WorldRect,
     ) -> (TileOffset, TileOffset) {
         // Translate the rectangle into the virtual tile space
         let origin = rect.origin - self.world_origin;
 
@@ -507,17 +553,17 @@ impl TileCache {
         } else {
             assert!(self.tiles.is_empty());
             self.tiles = mem::replace(&mut retained_tiles.tiles, Vec::new());
 
             // Get the positions of the reference primitives for this
             // new display list.
             let mut new_prim_map = FastHashMap::default();
             build_ref_prims(
-                &self.reference_prims,
+                &self.reference_prims.ref_prims,
                 &mut new_prim_map,
                 frame_context.clip_scroll_tree,
             );
 
             // Attempt to correlate them to work out which offset to apply.
             correlate_prim_maps(
                 &retained_tiles.ref_prims,
                 &new_prim_map,
@@ -651,17 +697,20 @@ impl TileCache {
         for y in 0 .. y_tiles {
             for x in 0 .. x_tiles {
                 let px = p0.x + x as f32 * TILE_SIZE_WIDTH as f32;
                 let py = p0.y + y as f32 * TILE_SIZE_HEIGHT as f32;
                 let key = (px.round() as i32, py.round() as i32);
 
                 let mut tile = match old_tiles.remove(&key) {
                     Some(tile) => tile,
-                    None => Tile::new(self.next_id()),
+                    None => {
+                        let next_id = TileId(NEXT_TILE_ID.fetch_add(1, Ordering::Relaxed));
+                        Tile::new(next_id)
+                    }
                 };
 
                 tile.world_rect = WorldRect::new(
                     WorldPoint::new(
                         px / frame_context.device_pixel_scale.0,
                         py / frame_context.device_pixel_scale.0,
                     ),
                     self.world_tile_size,
@@ -1102,28 +1151,31 @@ impl TileCache {
                 // assuming that it's either visible or we want to retain it for
                 // a while in case it gets scrolled back onto screen soon.
                 // TODO(gw): Consider switching to manual eviction policy?
                 resource_cache.texture_cache.request(&tile.handle, gpu_cache);
             } else {
                 tile.is_valid = false;
             }
 
+            // Invalidate the tile based on the content changing.
+            tile.update_content_validity();
+
             let visible_rect = match tile.visible_rect {
                 Some(rect) => rect,
                 None => continue,
             };
 
-            // Check the content of the tile is the same
+            // Check the valid rect of the primitive is sufficient.
             let tile_bounding_rect = match visible_rect.intersection(&self.world_bounding_rect) {
                 Some(rect) => rect.translate(&-tile.world_rect.origin.to_vector()),
                 None => continue,
             };
 
-            tile.update_validity(&tile_bounding_rect);
+            tile.update_rect_validity(&tile_bounding_rect);
 
             // If there are no primitives there is no need to draw or cache it.
             if tile.descriptor.prims.is_empty() {
                 continue;
             }
 
             // Decide how to handle this tile when drawing this frame.
             if tile.is_valid {
@@ -1135,23 +1187,23 @@ impl TileCache {
                         let tile_device_rect = tile.world_rect * frame_context.device_pixel_scale;
                         let mut label_pos = tile_device_rect.origin + DeviceVector2D::new(20.0, 30.0);
                         _scratch.push_debug_rect(
                             tile_device_rect,
                             debug_colors::GREEN,
                         );
                         _scratch.push_debug_string(
                             label_pos,
-                            debug_colors::WHITE,
-                            format!("{:?}", tile.id),
+                            debug_colors::RED,
+                            format!("{:?} {:?}", tile.id, tile.handle),
                         );
                         label_pos.y += 20.0;
                         _scratch.push_debug_string(
                             label_pos,
-                            debug_colors::WHITE,
+                            debug_colors::RED,
                             format!("same: {} frames", tile.same_frames),
                         );
                     }
                 }
             } else {
                 // Add the tile rect to the dirty rect.
                 dirty_world_rect = dirty_world_rect.union(&visible_rect);
 
@@ -1746,17 +1798,17 @@ impl PicturePrimitive {
         mut self,
         retained_tiles: &mut RetainedTiles,
         clip_scroll_tree: &ClipScrollTree,
     ) {
         if let Some(tile_cache) = self.tile_cache.take() {
             // Calculate and store positions of the reference
             // primitives for this tile cache.
             build_ref_prims(
-                &tile_cache.reference_prims,
+                &tile_cache.reference_prims.ref_prims,
                 &mut retained_tiles.ref_prims,
                 clip_scroll_tree,
             );
 
             for tile in tile_cache.tiles {
                 retained_tiles.tiles.push(tile);
             }
         }
@@ -2868,25 +2920,19 @@ fn correlate_prim_maps(
     }
 
     // Calculate the mode (the most common frequency of offset). This
     // can be different for some primitives, if they've animated, or
     // are attached to a different scroll node etc.
     map.into_iter()
         .max_by_key(|&(_, count)| count)
         .and_then(|(offset, count)| {
-            // We will assume we can use the calculated offset if:
-            // (a) We found more than one quarter of the selected
-            //     reference primitives to have the same offset.
-            // (b) The display lists both had the same number of
-            //     primitives, and we exactly matched. This handles
-            //     edge cases like scenes where there are very
-            //     few primitives, while excluding edge cases like
-            //     dl_mutate that have thousands of primitives with
-            //     the same uid.
-            if (count >= MIN_PRIMS_TO_CORRELATE) ||
-               (count == old_prims.len() && count == new_prims.len()) {
+            // We will assume we can use the calculated offset if we
+            // found more than one quarter of the selected reference
+            // primitives to have the same offset.
+            let prims_available = new_prims.len().min(old_prims.len());
+            if count >= prims_available / 4 {
                 Some(offset.into())
             } else {
                 None
             }
         })
 }
--- a/gfx/wr/webrender/src/renderer.rs
+++ b/gfx/wr/webrender/src/renderer.rs
@@ -4246,17 +4246,17 @@ impl Renderer {
         let debug_renderer = match self.debug.get_mut(&mut self.device) {
             Some(render) => render,
             None => return,
         };
 
         for item in items {
             match item {
                 DebugItem::Rect { rect, color } => {
-                    let inner_color = color.scale_alpha(0.2).into();
+                    let inner_color = color.scale_alpha(0.1).into();
                     let outer_color = (*color).into();
 
                     debug_renderer.add_quad(
                         rect.origin.x,
                         rect.origin.y,
                         rect.origin.x + rect.size.width,
                         rect.origin.y + rect.size.height,
                         inner_color,