Backed out changeset a900282391e1 (bug 1641751) for causing bugs 1643310, 1643238, 1643240. CLOSED TREE
authorButkovits Atila <abutkovits@mozilla.com>
Thu, 04 Jun 2020 15:41:14 +0300
changeset 533887 0d21bdf3fc0118b553c6543ddbff927a2c70cb96
parent 533886 aaee59d79784a0861b9d8d771fb261f2aff4acea
child 533888 e7e13672edd73c5f93516b2e1d65779f0f89004f
push id37479
push userapavel@mozilla.com
push dateThu, 04 Jun 2020 15:32:20 +0000
treeherdermozilla-central@0d21bdf3fc01 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1641751, 1643310, 1643238, 1643240
milestone79.0a1
backs outa900282391e15be8a8f9ed9782a550ff6be11cc7
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
Backed out changeset a900282391e1 (bug 1641751) for causing bugs 1643310, 1643238, 1643240. CLOSED TREE
gfx/wr/webrender/src/render_task_cache.rs
gfx/wr/webrender/src/texture_cache.rs
--- a/gfx/wr/webrender/src/render_task_cache.rs
+++ b/gfx/wr/webrender/src/render_task_cache.rs
@@ -142,27 +142,35 @@ impl RenderTaskCache {
             size.width,
             size.height,
             image_format,
             flags,
         );
 
         // Allocate space in the texture cache, but don't supply
         // and CPU-side data to be uploaded.
+        //
+        // Note that we currently use Eager eviction for cached render
+        // tasks, which means that any cached item not used in the last
+        // frame is discarded. There's room to be a lot smarter here,
+        // especially by considering the relative costs of re-rendering
+        // each type of item (box shadow blurs are an order of magnitude
+        // more expensive than borders, for example). Telemetry could
+        // inform our decisions here as well.
         texture_cache.update(
             &mut entry.handle,
             descriptor,
             TextureFilter::Linear,
             None,
             entry.user_data.unwrap_or([0.0; 3]),
             DirtyRect::All,
             gpu_cache,
             None,
             render_task.uv_rect_kind(),
-            Eviction::Auto,
+            Eviction::Eager,
         );
 
         // Get the allocation details in the texture cache, and store
         // this in the render task. The renderer will draw this
         // task into the appropriate layer and rect of the texture
         // cache on this frame.
         let (texture_id, texture_layer, uv_rect, _, _) =
             texture_cache.get_cache_location(&entry.handle);
--- a/gfx/wr/webrender/src/texture_cache.rs
+++ b/gfx/wr/webrender/src/texture_cache.rs
@@ -33,31 +33,23 @@ const PICTURE_TEXTURE_ADD_SLICES: usize 
 
 /// The chosen image format for picture tiles.
 const PICTURE_TILE_FORMAT: ImageFormat = ImageFormat::RGBA8;
 
 /// The number of pixels in a region. Derived from the above.
 const TEXTURE_REGION_PIXELS: usize =
     (TEXTURE_REGION_DIMENSIONS as usize) * (TEXTURE_REGION_DIMENSIONS as usize);
 
-/// If the total bytes allocated in shared / standalone cache is less
-/// than this, then allow the cache to grow without forcing an eviction.
-// TODO(gw): In future, it's probably reasonable to make this higher again, perhaps 64-128 MB.
-const CACHE_EVICTION_THRESHOLD_BYTES: usize = 32 * 1024 * 1024;
-
 /// Items in the texture cache can either be standalone textures,
 /// or a sub-rect inside the shared cache.
 #[derive(Debug)]
 #[cfg_attr(feature = "capture", derive(Serialize))]
 #[cfg_attr(feature = "replay", derive(Deserialize))]
 enum EntryDetails {
-    Standalone {
-        /// Number of bytes this entry allocates
-        size_in_bytes: usize,
-    },
+    Standalone,
     Picture {
         // Index in the picture_textures array
         texture_index: usize,
         // Slice in the texture array
         layer_index: usize,
     },
     Cache {
         /// Origin within the texture layer where this item exists.
@@ -65,28 +57,28 @@ enum EntryDetails {
         /// The layer index of the texture array.
         layer_index: usize,
     },
 }
 
 impl EntryDetails {
     fn describe(&self) -> (LayerIndex, DeviceIntPoint) {
         match *self {
-            EntryDetails::Standalone { .. }  => (0, DeviceIntPoint::zero()),
+            EntryDetails::Standalone => (0, DeviceIntPoint::zero()),
             EntryDetails::Picture { layer_index, .. } => (layer_index, DeviceIntPoint::zero()),
-            EntryDetails::Cache { origin, layer_index, .. } => (layer_index, origin),
+            EntryDetails::Cache { origin, layer_index } => (layer_index, origin),
         }
     }
 }
 
 impl EntryDetails {
     /// Returns the kind associated with the details.
     fn kind(&self) -> EntryKind {
         match *self {
-            EntryDetails::Standalone { .. } => EntryKind::Standalone,
+            EntryDetails::Standalone => EntryKind::Standalone,
             EntryDetails::Picture { .. } => EntryKind::Picture,
             EntryDetails::Cache { .. } => EntryKind::Shared,
         }
     }
 }
 
 /// Tag identifying standalone-versus-shared, without the details.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -132,25 +124,22 @@ struct CacheEntry {
 
 impl CacheEntry {
     // Create a new entry for a standalone texture.
     fn new_standalone(
         texture_id: CacheTextureId,
         last_access: FrameStamp,
         params: &CacheAllocParams,
         swizzle: Swizzle,
-        size_in_bytes: usize,
     ) -> Self {
         CacheEntry {
             size: params.descriptor.size,
             user_data: params.user_data,
             last_access,
-            details: EntryDetails::Standalone {
-                size_in_bytes,
-            },
+            details: EntryDetails::Standalone,
             texture_id,
             input_format: params.descriptor.format,
             filter: params.filter,
             swizzle,
             uv_rect_handle: GpuCacheHandle::new(),
             eviction_notice: None,
             uv_rect_kind: params.uv_rect_kind,
             eviction: Eviction::Auto,
@@ -205,16 +194,20 @@ pub type TextureCacheHandle = WeakFreeLi
 #[cfg_attr(feature = "replay", derive(Deserialize))]
 pub enum Eviction {
     /// The entry will be evicted under the normal rules (which differ between
     /// standalone and shared entries).
     Auto,
     /// The entry will not be evicted until the policy is explicitly set to a
     /// different value.
     Manual,
+    /// The entry will be evicted if it was not used in the last frame.
+    ///
+    /// FIXME(bholley): Currently this only applies to the standalone case.
+    Eager,
 }
 
 // An eviction notice is a shared condition useful for detecting
 // when a TextureCacheHandle gets evicted from the TextureCache.
 // It is optionally installed to the TextureCache when an update()
 // is scheduled. A single notice may be shared among any number of
 // TextureCacheHandle updates. The notice may then be subsequently
 // checked to see if any of the updates using it have been evicted.
@@ -632,30 +625,23 @@ pub struct TextureCache {
     pending_updates: TextureUpdateList,
 
     /// The current `FrameStamp`. Used for cache eviction policies.
     now: FrameStamp,
 
     /// Maintains the list of all current items in the texture cache.
     entries: FreeList<CacheEntry, CacheEntryMarker>,
 
-    /// The last `FrameStamp` in which we expired the shared cache.
+    /// The last `FrameStamp` in which we expired the shared cache for
+    /// this document.
     last_shared_cache_expiration: FrameStamp,
 
     /// Strong handles for all entries that this document has allocated
     /// from the shared FreeList.
     handles: EntryHandles,
-
-    /// Estimated memory usage of allocated entries in all of the shared textures. This
-    /// is used to decide when to evict old items from the cache.
-    shared_bytes_allocated: usize,
-
-    /// Number of bytes allocated in standalone textures. Used as an input to deciding
-    /// when to run texture cache eviction.
-    standalone_bytes_allocated: usize,
 }
 
 impl TextureCache {
     pub fn new(
         max_texture_size: i32,
         mut max_texture_layers: usize,
         picture_tile_sizes: &[DeviceIntSize],
         initial_size: DeviceIntSize,
@@ -714,18 +700,16 @@ impl TextureCache {
             max_texture_layers,
             swizzle,
             debug_flags: DebugFlags::empty(),
             next_id: next_texture_id,
             pending_updates,
             now: FrameStamp::INVALID,
             last_shared_cache_expiration: FrameStamp::INVALID,
             handles: EntryHandles::default(),
-            shared_bytes_allocated: 0,
-            standalone_bytes_allocated: 0,
         }
     }
 
     /// Creates a TextureCache and sets it up with a valid `FrameStamp`, which
     /// is useful for avoiding panics when instantiating the `TextureCache`
     /// directly from unit test code.
     #[cfg(test)]
     pub fn new_for_testing(
@@ -804,17 +788,17 @@ impl TextureCache {
         // Most of the time, standalone cache entries correspond to images whose
         // width or height is greater than the region size in the shared cache, i.e.
         // 512 pixels. Cached render tasks also frequently get standalone entries,
         // but those use the Eviction::Eager policy (for now). So the tradeoff there
         // is largely around reducing texture upload jank while keeping memory usage
         // at an acceptable level.
         let threshold = self.default_eviction();
         self.expire_old_entries(EntryKind::Standalone, threshold);
-        self.expire_old_picture_cache_tiles();
+        self.expire_old_entries(EntryKind::Picture, threshold);
 
         self.shared_textures.array_alpha8_linear.release_empty_textures(&mut self.pending_updates);
         self.shared_textures.array_alpha16_linear.release_empty_textures(&mut self.pending_updates);
         self.shared_textures.array_color8_linear.release_empty_textures(&mut self.pending_updates);
         self.shared_textures.array_color8_nearest.release_empty_textures(&mut self.pending_updates);
 
         self.shared_textures.array_alpha8_linear
             .update_profile(&mut texture_cache_profile.pages_alpha8_linear);
@@ -1051,66 +1035,45 @@ impl TextureCache {
     fn default_eviction(&self) -> EvictionThreshold {
         EvictionThresholdBuilder::new(self.now)
             .max_frames(200)
             .max_time_s(2)
             .scale_by_pressure()
             .build()
     }
 
-    /// Expire picture cache tiles that haven't been referenced in the last frame.
-    /// The picture cache code manually keeps tiles alive by calling `request` on
-    /// them if it wants to retain a tile that is currently not visible.
-    fn expire_old_picture_cache_tiles(&mut self) {
-        for i in (0 .. self.handles.picture.len()).rev() {
-            let evict = {
-                let entry = self.entries.get(&self.handles.picture[i]);
-
-                // Texture cache entries can be evicted at the start of
-                // a frame, or at any time during the frame when a cache
-                // allocation is occurring. This means that entries tagged
-                // with eager eviction may get evicted before they have a
-                // chance to be requested on the current frame. Instead,
-                // advance the frame id of the entry by one before
-                // comparison. This ensures that an eager entry will
-                // not be evicted until it is not used for at least
-                // one complete frame.
-                let mut entry_frame_id = entry.last_access.frame_id();
-                entry_frame_id.advance();
-
-                entry_frame_id < self.now.frame_id()
-            };
-
-            if evict {
-                let handle = self.handles.picture.swap_remove(i);
-                let entry = self.entries.free(handle);
-                entry.evict();
-                self.free(&entry);
-            }
-        }
-    }
-
     /// Shared eviction code for standalone and shared entries.
     ///
     /// See `EvictionThreshold` for more details on policy.
     fn expire_old_entries(&mut self, kind: EntryKind, threshold: EvictionThreshold) {
-        // Should not be used to expire picture tiles. This entire method will be
-        // removed soon, in favor of proper LRU eviction for shared / standalone entries.
-        debug_assert_ne!(kind, EntryKind::Picture);
-
         debug_assert!(self.now.is_valid());
         // Iterate over the entries in reverse order, evicting the ones older than
         // the frame age threshold. Reverse order avoids iterator invalidation when
         // removing entries.
         for i in (0..self.handles.select(kind).len()).rev() {
             let evict = {
                 let entry = self.entries.get(&self.handles.select(kind)[i]);
                 match entry.eviction {
                     Eviction::Manual => false,
                     Eviction::Auto => threshold.should_evict(entry.last_access),
+                    Eviction::Eager => {
+                        // Texture cache entries can be evicted at the start of
+                        // a frame, or at any time during the frame when a cache
+                        // allocation is occurring. This means that entries tagged
+                        // with eager eviction may get evicted before they have a
+                        // chance to be requested on the current frame. Instead,
+                        // advance the frame id of the entry by one before
+                        // comparison. This ensures that an eager entry will
+                        // not be evicted until it is not used for at least
+                        // one complete frame.
+                        let mut entry_frame_id = entry.last_access.frame_id();
+                        entry_frame_id.advance();
+
+                        entry_frame_id < self.now.frame_id()
+                    }
                 }
             };
             if evict {
                 let handle = self.handles.select(kind).swap_remove(i);
                 let entry = self.entries.free(handle);
                 entry.evict();
                 self.free(&entry);
             }
@@ -1123,25 +1086,16 @@ impl TextureCache {
     fn maybe_expire_old_shared_entries(&mut self, threshold: EvictionThreshold) {
         debug_assert!(self.now.is_valid());
         if self.last_shared_cache_expiration.frame_id() < self.now.frame_id() {
             self.expire_old_entries(EntryKind::Shared, threshold);
             self.last_shared_cache_expiration = self.now;
         }
     }
 
-    /// Return the total used bytes in standalone and shared textures. This is
-    /// used to determine how many textures need to be evicted to keep texture
-    /// cache memory usage under a reasonable limit. Note that this does not
-    /// include memory allocated to picture cache tiles, which are considered
-    /// separately for the purposes of texture cache eviction.
-    fn current_memory_estimate(&self) -> usize {
-        self.standalone_bytes_allocated + self.shared_bytes_allocated
-    }
-
     // Free a cache entry from the standalone list or shared cache.
     fn free(&mut self, entry: &CacheEntry) {
         match entry.details {
             EntryDetails::Picture { texture_index, layer_index } => {
                 let picture_texture = self.picture_textures.get(texture_index);
                 picture_texture.slices[layer_index].uv_rect_handle = None;
                 if self.debug_flags.contains(
                     DebugFlags::TEXTURE_CACHE_DBG |
@@ -1151,33 +1105,29 @@ impl TextureCache {
                         entry.texture_id,
                         DeviceIntPoint::zero(),
                         picture_texture.size.width,
                         picture_texture.size.height,
                         layer_index,
                     );
                 }
             }
-            EntryDetails::Standalone { size_in_bytes, .. } => {
-                self.standalone_bytes_allocated -= size_in_bytes;
-
+            EntryDetails::Standalone => {
                 // This is a standalone texture allocation. Free it directly.
                 self.pending_updates.push_free(entry.texture_id);
             }
-            EntryDetails::Cache { origin, layer_index, .. } => {
+            EntryDetails::Cache { origin, layer_index } => {
                 // Free the block in the given region.
                 let texture_array = self.shared_textures.select(entry.input_format, entry.filter);
                 let unit = texture_array.units
                     .iter_mut()
                     .find(|unit| unit.texture_id == entry.texture_id)
                     .expect("Unable to find the associated texture array unit");
                 let region = &mut unit.regions[layer_index];
 
-                self.shared_bytes_allocated -= region.slab_size.size_in_bytes(texture_array.formats.internal);
-
                 if self.debug_flags.contains(
                     DebugFlags::TEXTURE_CACHE_DBG |
                     DebugFlags::TEXTURE_CACHE_DBG_CLEAR_EVICTED)
                 {
                     self.pending_updates.push_debug_clear(
                         entry.texture_id,
                         origin,
                         region.slab_size.width,
@@ -1267,18 +1217,16 @@ impl TextureCache {
             unit.push_regions(texture_array.layers_per_allocation);
 
             info.layer_count = unit.regions.len() as i32;
             self.pending_updates.push_alloc(self.next_id, info);
             self.next_id.0 += 1;
             index
         };
 
-        self.shared_bytes_allocated += slab_size.size_in_bytes(texture_array.formats.internal);
-
         // Do the allocation. This can fail and return None
         // if there are no free slots or regions available.
         texture_array.alloc(params, unit_index, self.now, swizzle)
     }
 
     // Returns true if the given image descriptor *may* be
     // placed in the shared texture cache.
     pub fn is_allowed_in_shared_cache(
@@ -1323,35 +1271,30 @@ impl TextureCache {
             width: params.descriptor.size.width,
             height: params.descriptor.size.height,
             format: params.descriptor.format,
             filter: params.filter,
             layer_count: 1,
             is_shared_cache: false,
             has_depth: false,
         };
-
-        let size_in_bytes = (info.width * info.height * info.format.bytes_per_pixel()) as usize;
-        self.standalone_bytes_allocated += size_in_bytes;
-
         self.pending_updates.push_alloc(texture_id, info);
 
         // Special handing for BGRA8 textures that may need to be swizzled.
         let swizzle = if params.descriptor.format == ImageFormat::BGRA8 {
             self.swizzle.map(|s| s.bgra8_sampling_swizzle)
         } else {
             None
         };
 
         CacheEntry::new_standalone(
             texture_id,
             self.now,
             params,
             swizzle.unwrap_or_default(),
-            size_in_bytes,
         )
     }
 
     /// Allocates a cache entry appropriate for the given parameters.
     ///
     /// This allocates from the shared cache unless the parameters do not meet
     /// the shared cache requirements, in which case a standalone texture is
     /// used.
@@ -1359,18 +1302,17 @@ impl TextureCache {
         &mut self,
         params: &CacheAllocParams,
     ) -> CacheEntry {
         assert!(!params.descriptor.size.is_empty_or_negative());
 
         // If this image doesn't qualify to go in the shared (batching) cache,
         // allocate a standalone entry.
         if self.is_allowed_in_shared_cache(params.filter, &params.descriptor) {
-            if !self.has_space_in_shared_cache(params) &&
-               self.current_memory_estimate() > CACHE_EVICTION_THRESHOLD_BYTES {
+            if !self.has_space_in_shared_cache(params) {
                 // If we don't have extra space and haven't GCed this frame, do so.
                 let threshold = self.default_eviction();
                 self.maybe_expire_old_shared_entries(threshold);
             }
             self.allocate_from_shared_cache(params)
         } else {
             self.allocate_standalone_entry(params)
         }
@@ -1494,21 +1436,16 @@ impl SlabSize {
         };
 
         SlabSize {
             width,
             height,
         }
     }
 
-    fn size_in_bytes(&self, format: ImageFormat) -> usize {
-        let bpp = format.bytes_per_pixel();
-        (self.width * self.height * bpp) as usize
-    }
-
     fn invalid() -> SlabSize {
         SlabSize {
             width: 0,
             height: 0,
         }
     }
 }
 
@@ -1836,17 +1773,17 @@ impl WholeTextureArray {
             },
             uv_rect_handle,
             input_format: self.format,
             filter: self.filter,
             swizzle: Swizzle::default(),
             texture_id,
             eviction_notice: None,
             uv_rect_kind: UvRectKind::Rect,
-            eviction: Eviction::Auto,
+            eviction: Eviction::Eager,
         }
     }
 
     /// Occupy a specified slice by a cache entry.
     fn occupy(
         &mut self,
         texture_index: usize,
         layer_index: usize,