Bug 1527498 - WR texture cache code cleanup r=jrmuizel
authorDzmitry Malyshau <dmalyshau@mozilla.com>
Wed, 13 Feb 2019 21:49:55 +0000
changeset 459066 84d60f483f42
parent 459065 bf288072dd8a
child 459067 5542b48b6dfe
push id111913
push usershindli@mozilla.com
push dateThu, 14 Feb 2019 05:01:59 +0000
treeherdermozilla-inbound@a0752d7e8073 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1527498
milestone67.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 1527498 - WR texture cache code cleanup r=jrmuizel This is a preparatory change that can be useful by itself: - use match on EntryKind to allow safe expansion - avoid code duplication in get() - fix some comments Differential Revision: https://phabricator.services.mozilla.com/D19674
gfx/wr/webrender/src/render_task.rs
gfx/wr/webrender/src/texture_cache.rs
--- a/gfx/wr/webrender/src/render_task.rs
+++ b/gfx/wr/webrender/src/render_task.rs
@@ -1247,17 +1247,17 @@ impl RenderTaskCache {
                     render_task.uv_rect_kind(),
                     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) =
+                let (texture_id, texture_layer, uv_rect, _) =
                     texture_cache.get_cache_location(&entry.handle);
 
                 render_task.location = RenderTaskLocation::TextureCache {
                     texture: texture_id,
                     layer: texture_layer,
                     rect: uv_rect.to_i32(),
                 };
             }
--- a/gfx/wr/webrender/src/texture_cache.rs
+++ b/gfx/wr/webrender/src/texture_cache.rs
@@ -30,19 +30,19 @@ const TEXTURE_REGION_PIXELS: usize =
 /// 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,
     Cache {
-        // Origin within the texture layer where this item exists.
+        /// Origin within the texture layer where this item exists.
         origin: DeviceIntPoint,
-        // The layer index of the texture array.
+        /// The layer index of the texture array.
         layer_index: usize,
     },
 }
 
 impl EntryDetails {
     /// Returns the kind associated with the details.
     fn kind(&self) -> EntryKind {
         match *self {
@@ -116,22 +116,18 @@ impl CacheEntry {
 
     // Update the GPU cache for this texture cache entry.
     // This ensures that the UV rect, and texture layer index
     // are up to date in the GPU cache for vertex shaders
     // to fetch from.
     fn update_gpu_cache(&mut self, gpu_cache: &mut GpuCache) {
         if let Some(mut request) = gpu_cache.request(&mut self.uv_rect_handle) {
             let (origin, layer_index) = match self.details {
-                EntryDetails::Standalone { .. } => (DeviceIntPoint::zero(), 0.0),
-                EntryDetails::Cache {
-                    origin,
-                    layer_index,
-                    ..
-                } => (origin, layer_index as f32),
+                EntryDetails::Standalone => (DeviceIntPoint::zero(), 0.0),
+                EntryDetails::Cache { origin, layer_index } => (origin, layer_index as f32),
             };
             let image_source = ImageSource {
                 p0: origin.to_f32(),
                 p1: (origin + self.size).to_f32(),
                 texture_layer: layer_index,
                 user_data: self.user_data,
                 uv_rect_kind: self.uv_rect_kind,
             };
@@ -290,20 +286,19 @@ struct EntryHandles {
     standalone: Vec<FreeListHandle<CacheEntryMarker>>,
     /// Handles for each shared texture cache entry.
     shared: Vec<FreeListHandle<CacheEntryMarker>>,
 }
 
 impl EntryHandles {
     /// Mutably borrows the requested handle list.
     fn select(&mut self, kind: EntryKind) -> &mut Vec<FreeListHandle<CacheEntryMarker>> {
-        if kind == EntryKind::Standalone {
-            &mut self.standalone
-        } else {
-            &mut self.shared
+        match kind {
+            EntryKind::Standalone => &mut self.standalone,
+            EntryKind::Shared => &mut self.shared,
         }
     }
 }
 
 /// Container struct for the various parameters used in cache allocation.
 struct CacheAllocParams {
     descriptor: ImageDescriptor,
     filter: TextureFilter,
@@ -806,22 +801,18 @@ impl TextureCache {
 
         entry.eviction = eviction;
 
         // Create an update command, which the render thread processes
         // to upload the new image data into the correct location
         // in GPU memory.
         if let Some(data) = data {
             let (layer_index, origin) = match entry.details {
-                EntryDetails::Standalone { .. } => (0, DeviceIntPoint::zero()),
-                EntryDetails::Cache {
-                    layer_index,
-                    origin,
-                    ..
-                } => (layer_index, origin),
+                EntryDetails::Standalone => (0, DeviceIntPoint::zero()),
+                EntryDetails::Cache { layer_index, origin } => (layer_index, origin),
             };
 
             let op = TextureCacheUpdate::new_update(
                 data,
                 &descriptor,
                 origin,
                 entry.size,
                 entry.texture_id,
@@ -839,63 +830,52 @@ impl TextureCache {
     }
 
     // Retrieve the details of an item in the cache. This is used
     // during batch creation to provide the resource rect address
     // to the shaders and texture ID to the batching logic.
     // This function will assert in debug modes if the caller
     // tries to get a handle that was not requested this frame.
     pub fn get(&self, handle: &TextureCacheHandle) -> CacheItem {
-        let entry = self.entries
-            .get_opt(handle)
-            .expect("BUG: was dropped from cache or not updated!");
-        debug_assert_eq!(entry.last_access, self.now);
-        let (layer_index, origin) = match entry.details {
-            EntryDetails::Standalone { .. } => {
-                (0, DeviceIntPoint::zero())
-            }
-            EntryDetails::Cache {
-                layer_index,
-                origin,
-                ..
-            } => (layer_index, origin),
-        };
+        let (texture_id, layer_index, uv_rect, uv_rect_handle) = self.get_cache_location(handle);
         CacheItem {
-            uv_rect_handle: entry.uv_rect_handle,
-            texture_id: TextureSource::TextureCache(entry.texture_id),
-            uv_rect: DeviceIntRect::new(origin, entry.size),
+            uv_rect_handle,
+            texture_id: TextureSource::TextureCache(texture_id),
+            uv_rect,
             texture_layer: layer_index as i32,
         }
     }
 
     /// A more detailed version of get(). This allows access to the actual
     /// device rect of the cache allocation.
     ///
-    /// Returns a tuple identifying the texture, the layer, and the region.
+    /// Returns a tuple identifying the texture, the layer, the region,
+    /// and its GPU handle.
     pub fn get_cache_location(
         &self,
         handle: &TextureCacheHandle,
-    ) -> (CacheTextureId, LayerIndex, DeviceIntRect) {
+    ) -> (CacheTextureId, LayerIndex, DeviceIntRect, GpuCacheHandle) {
         let entry = self.entries
             .get_opt(handle)
             .expect("BUG: was dropped from cache or not updated!");
         debug_assert_eq!(entry.last_access, self.now);
         let (layer_index, origin) = match entry.details {
             EntryDetails::Standalone { .. } => {
                 (0, DeviceIntPoint::zero())
             }
             EntryDetails::Cache {
                 layer_index,
                 origin,
                 ..
             } => (layer_index, origin),
         };
         (entry.texture_id,
          layer_index as usize,
-         DeviceIntRect::new(origin, entry.size))
+         DeviceIntRect::new(origin, entry.size),
+         entry.uv_rect_handle)
     }
 
     pub fn mark_unused(&mut self, handle: &TextureCacheHandle) {
         if let Some(entry) = self.entries.get_opt_mut(handle) {
             // Set last accessed stamp invalid to ensure it gets cleaned up
             // next time we expire entries.
             entry.last_access = FrameStamp::INVALID;
             entry.eviction = Eviction::Auto;
@@ -975,24 +955,21 @@ impl TextureCache {
             self.doc_data.last_shared_cache_expiration = self.now;
         }
         self.doc_data.handles.shared.len() != old_len
     }
 
     // Free a cache entry from the standalone list or shared cache.
     fn free(&mut self, entry: CacheEntry) {
         match entry.details {
-            EntryDetails::Standalone { .. } => {
+            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.format, entry.filter);
                 let region = &mut texture_array.regions[layer_index];
 
                 if self.debug_flags.contains(
                     DebugFlags::TEXTURE_CACHE_DBG |
                     DebugFlags::TEXTURE_CACHE_DBG_CLEAR_EVICTED) {
                     self.pending_updates.push_debug_clear(
@@ -1197,20 +1174,21 @@ impl TextureCache {
         //
         // This is managed with a database style upsert operation.
         match self.entries.upsert(handle, new_cache_entry) {
             UpsertResult::Updated(old_entry) => {
                 if new_kind != old_entry.details.kind() {
                     // Handle the rare case than an update moves an entry from
                     // shared to standalone or vice versa. This involves a linear
                     // search, but should be rare enough not to matter.
-                    let (from, to) = if new_kind == EntryKind::Standalone {
-                        (&mut self.doc_data.handles.shared, &mut self.doc_data.handles.standalone)
-                    } else {
-                        (&mut self.doc_data.handles.standalone, &mut self.doc_data.handles.shared)
+                    let (from, to) = match new_kind {
+                        EntryKind::Standalone =>
+                            (&mut self.doc_data.handles.shared, &mut self.doc_data.handles.standalone),
+                        EntryKind::Shared =>
+                            (&mut self.doc_data.handles.standalone, &mut self.doc_data.handles.shared),
                     };
                     let idx = from.iter().position(|h| h.weak() == *handle).unwrap();
                     to.push(from.remove(idx));
                 }
                 self.free(old_entry);
             }
             UpsertResult::Inserted(new_handle) => {
                 *handle = new_handle.weak();