Bug 1618319: Segregate intern::UpdateList insertions and removals. r=gw

Rather than treating webrender::intern::UpdateList as a sequence of operations,
each of which might be an insertion or a removal, and using a side table to
supply extra data for insertions, it's simpler to segregate insertions and
removals into two separate vectors. This avoids the need for an enum whose
discriminant needs to be checked within the loop, and allows a few loops that
are only looking for one kind of operation to skip over the others entirely.

Ultimately, there should be no change in the order in which operations occur. In
practice, the old UpdateList always held a contiguous run of insertions,
followed by a run of removals (removals are consumed by apply_updates directly
after being generated by end_frame_and_get_pending_updates).

Differential Revision: https://phabricator.services.mozilla.com/D64444

--HG--
extra : moz-landing-system : lando

gfx/wr/tileview/src/main.rs

@@ -15,7 +15,7 @@
 
 use webrender::{TileNode, TileNodeKind, InvalidationReason, TileOffset};
 use webrender::{TileSerializer, TileCacheInstanceSerializer, TileCacheLoggerUpdateLists};
-use webrender::{PrimitiveCompareResultDetail, CompareHelperResult, UpdateKind, ItemUid};
+use webrender::{PrimitiveCompareResultDetail, CompareHelperResult, ItemUid};
 use serde::Deserialize;
 use std::fs::File;
 use std::io::prelude::*;
@@ -516,20 +516,15 @@ macro_rules! updatelist_to_html_macro {
                 html += &format!("<div class=\"subheader\">{}</div>\n<div class=\"intern data\">\n",
                                  stringify!($name));
                 for list in &update_lists.$name.1 {
-                    let mut insert_count = 0;
-                    for update in &list.updates {
-                        match update.kind {
-                            UpdateKind::Insert => {
-                                html += &format!("<div class=\"insert\"><b>{}</b> {}</div>\n",
-                                                 update.uid.get_uid(),
-                                                 format!("({:?})", list.data[insert_count]));
-                                insert_count = insert_count + 1;
-                            }
-                            _ => {
-                                html += &format!("<div class=\"remove\"><b>{}</b></div>\n",
-                                                 update.uid.get_uid());
-                            }
-                        };
+                    for insertion in &list.insertions {
+                        html += &format!("<div class=\"insert\"><b>{}</b> {}</div>\n",
+                                         insertion.uid.get_uid(),
+                                         format!("({:?})", insertion.value));
+                    }
+
+                    for removal in &list.removals {
+                        html += &format!("<div class=\"remove\"><b>{}</b></div>\n",
+                                         removal.uid.get_uid());
                     }
                 }
                 html += "</div><br/>\n";

gfx/wr/webrender/src/intern.rs

@@ -52,11 +52,46 @@ struct Epoch(u64);
 /// provided by the interning structure.
 #[cfg_attr(feature = "capture", derive(Serialize))]
 #[cfg_attr(feature = "replay", derive(Deserialize))]
+#[derive(MallocSizeOf)]
 pub struct UpdateList<S> {
-    /// The additions and removals to apply.
-    pub updates: Vec<Update>,
-    /// Actual new data to insert.
-    pub data: Vec<S>,
+    /// Items to insert.
+    pub insertions: Vec<Insertion<S>>,
+
+    /// Items to remove.
+    pub removals: Vec<Removal>,
+}
+
+#[cfg_attr(feature = "capture", derive(Serialize))]
+#[cfg_attr(feature = "replay", derive(Deserialize))]
+#[derive(MallocSizeOf)]
+pub struct Insertion<S> {
+    pub index: usize,
+    pub uid: ItemUid,
+    pub value: S,
+}
+
+#[cfg_attr(feature = "capture", derive(Serialize))]
+#[cfg_attr(feature = "replay", derive(Deserialize))]
+#[derive(MallocSizeOf)]
+pub struct Removal {
+    pub index: usize,
+    pub uid: ItemUid,
+}
+
+impl<S> UpdateList<S> {
+    fn new() -> UpdateList<S> {
+        UpdateList {
+            insertions: Vec::new(),
+            removals: Vec::new(),
+        }
+    }
+
+    fn take_and_preallocate(&mut self) -> UpdateList<S> {
+        UpdateList {
+            insertions: self.insertions.take_and_preallocate(),
+            removals: self.removals.take_and_preallocate(),
+        }
+    }
 }
 
 lazy_static! {
@@ -112,23 +147,6 @@ impl<I> Handle<I> {
     }
 }
 
-#[cfg_attr(feature = "capture", derive(Serialize))]
-#[cfg_attr(feature = "replay", derive(Deserialize))]
-#[derive(MallocSizeOf)]
-pub enum UpdateKind {
-    Insert,
-    Remove,
-}
-
-#[cfg_attr(feature = "capture", derive(Serialize))]
-#[cfg_attr(feature = "replay", derive(Deserialize))]
-#[derive(MallocSizeOf)]
-pub struct Update {
-    pub index: usize,
-    pub uid: ItemUid,
-    pub kind: UpdateKind,
-}
-
 pub trait InternDebug {
     fn on_interned(&self, _uid: ItemUid) {}
 }
@@ -158,25 +176,18 @@ impl<I: Internable> DataStore<I> {
         update_list: UpdateList<I::Key>,
         profile_counter: &mut ResourceProfileCounter,
     ) {
-        let mut data_iter = update_list.data.into_iter();
-        for update in update_list.updates {
-            match update.kind {
-                UpdateKind::Insert => {
-                    let value = data_iter.next().unwrap().into();
-                    self.items
-                        .entry(update.index)
-                        .set(Some(value));
-                }
-                UpdateKind::Remove => {
-                    self.items[update.index] = None;
-                }
-            }
+        for insertion in update_list.insertions {
+            self.items
+                .entry(insertion.index)
+                .set(Some(insertion.value.into()));
+        }
+
+        for removal in update_list.removals {
+            self.items[removal.index] = None;
         }
 
         let per_item_size = mem::size_of::<I::Key>() + mem::size_of::<I::StoreData>();
         profile_counter.set(self.items.len(), per_item_size * self.items.len());
-
-        debug_assert!(data_iter.next().is_none());
     }
 }
 
@@ -210,9 +221,7 @@ pub struct Interner<I: Internable> {
     /// List of free slots in the data store for re-use.
     free_list: Vec<usize>,
     /// Pending list of updates that need to be applied.
-    updates: Vec<Update>,
-    /// Pending new data to insert.
-    update_data: Vec<I::Key>,
+    update_list: UpdateList<I::Key>,
     /// The current epoch for the interner.
     current_epoch: Epoch,
     /// The information associated with each interned
@@ -225,8 +234,7 @@ impl<I: Internable> Default for Interner<I> {
         Interner {
             map: FastHashMap::default(),
             free_list: Vec::new(),
-            updates: Vec::new(),
-            update_data: Vec::new(),
+            update_list: UpdateList::new(),
             current_epoch: Epoch(1),
             local_data: Vec::new(),
         }
@@ -265,12 +273,11 @@ impl<I: Internable> Interner<I> {
         let uid = ItemUid::next_uid();
 
         // Add a pending update to insert the new data.
-        self.updates.push(Update {
+        self.update_list.insertions.push(Insertion {
             index,
             uid,
-            kind: UpdateKind::Insert,
+            value: data.clone(),
         });
-        self.update_data.alloc().init(data.clone());
 
         // Generate a handle for access via the data store.
         let handle = Handle {
@@ -298,8 +305,7 @@ impl<I: Internable> Interner<I> {
     /// that need to be applied to the data store. Also run
     /// a GC step that removes old entries.
     pub fn end_frame_and_get_pending_updates(&mut self) -> UpdateList<I::Key> {
-        let mut updates = self.updates.take_and_preallocate();
-        let data = self.update_data.take_and_preallocate();
+        let mut update_list = self.update_list.take_and_preallocate();
 
         let free_list = &mut self.free_list;
         let current_epoch = self.current_epoch.0;
@@ -315,28 +321,23 @@ impl<I: Internable> Interner<I> {
             if handle.epoch.0 + 10 < current_epoch {
                 // To expire an item:
                 //  - Add index to the free-list for re-use.
-                //  - Add an update to the data store to invalidate this slow.
+                //  - Add an update to the data store to invalidate this slot.
                 //  - Remove from the hash map.
                 free_list.push(handle.index as usize);
-                updates.push(Update {
+                update_list.removals.push(Removal {
                     index: handle.index as usize,
                     uid: handle.uid,
-                    kind: UpdateKind::Remove,
                 });
                 return false;
             }
 
             true
         });
-        let updates = UpdateList {
-            updates,
-            data,
-        };
 
         // Begin the next epoch
         self.current_epoch = Epoch(self.current_epoch.0 + 1);
 
-        updates
+        update_list
     }
 }
 

gfx/wr/webrender/src/lib.rs

@@ -224,4 +224,4 @@ pub use webrender_build::shader::ProgramSourceDigest;
 pub use crate::picture::{TileDescriptor, TileId, InvalidationReason};
 pub use crate::picture::{PrimitiveCompareResult, PrimitiveCompareResultDetail, CompareHelperResult};
 pub use crate::picture::{TileNode, TileNodeKind, TileSerializer, TileCacheInstanceSerializer, TileOffset, TileCacheLoggerUpdateLists};
-pub use crate::intern::{Update, UpdateKind, ItemUid};
+pub use crate::intern::ItemUid;

gfx/wr/webrender/src/picture.rs

@@ -141,8 +141,6 @@ use ron;
 use crate::scene_builder_thread::InternerUpdates;
 #[cfg(any(feature = "capture", feature = "replay"))]
 use crate::intern::{Internable, UpdateList};
-#[cfg(any(feature = "replay"))]
-use crate::intern::{UpdateKind};
 #[cfg(any(feature = "capture", feature = "replay"))]
 use api::{ClipIntern, FilterDataIntern, PrimitiveKeyKind};
 #[cfg(any(feature = "capture", feature = "replay"))]
@@ -1947,17 +1945,10 @@ macro_rules! declare_tile_cache_logger_updatelists {
                 $(
                     {
                         for list in &self.$name.1 {
-                            let mut insert_count = 0;
-                            for update in &list.updates {
-                                match update.kind {
-                                    UpdateKind::Insert => {
-                                        itemuid_to_string.insert(
-                                            update.uid,
-                                            format!("{:?}", list.data[insert_count]));
-                                        insert_count = insert_count + 1;
-                                    },
-                                    _ => {}
-                                }
+                            for insertion in &list.insertions {
+                                itemuid_to_string.insert(
+                                    insertion.uid,
+                                    format!("{:?}", insertion.value));
                             }
                         }
                     }