Cycles: optimize attributes device updates

When an `AttributeSet` is tagged as modified, which happens after the addition or
removal of an `Attribute` from the set, during the following GeometryManager device
update, we update and repack the kernel data for all attribute types. However, if we
only add or remove a `float` attribute, `float2` or `float3` attributes should not
be repacked for efficiency.

This patch adds some mechanisms to detect which attribute types are modified from
the AttributeSet.

Firstly, this adds an `AttrKernelDataType` to map the data type of the Attribute to
the one used in the kernel as there is no one to one match between the two since e.g.
`Transform` or `float4` data are stored as `float3s` in the kernel.

Then, this replaces the `AttributeSet.modified` boolean with a set of flags to detect
which types have been modified. There is no specific flag type (e.g.
`enum ModifiedType`), rather the flags used derive simply from the
`AttrKernelDataType` enumeration, to keep things synchronized.

The logic to remove an `Attribute` from the `AttributeSet` and tag the latter as modified
is centralized in a new `AttributeSet.remove` method taking an iterator as input.

Lastly, as some attributes like standard normals are not stored in the various
kernel attribute arrays (`DeviceScene::attribute_*`), the modified flags are only
set if the associated standard corresponds to an attribute which will be stored
in the kernel's attribute arrays. This makes it so adding or removing such attributes
does not trigger an unnecessary update of other type-related attributes.

Reviewed By: brecht

Differential Revision: https://developer.blender.org/D11373

4 files changed, 138 insertions, 45 deletions

diff --git a/intern/cycles/render/alembic.cpp b/intern/cycles/render/alembic.cpp
index cf345ee075d..dcb456dc1ce 100644
--- a/intern/cycles/render/alembic.cpp
+++ b/intern/cycles/render/alembic.cpp
@@ -654,8 +654,7 @@ static void update_attributes(AttributeSet &attributes, CachedData &cached_data,
   list<Attribute>::iterator it;
   for (it = attributes.attributes.begin(); it != attributes.attributes.end();) {
     if (cached_attributes.find(&(*it)) == cached_attributes.end()) {
-      attributes.attributes.erase(it++);
-      attributes.modified = true;
+      attributes.remove(it++);
       continue;
     }
 
diff --git a/intern/cycles/render/attribute.cpp b/intern/cycles/render/attribute.cpp
index d6a638fd4cd..bf9d69cb47e 100644
--- a/intern/cycles/render/attribute.cpp
+++ b/intern/cycles/render/attribute.cpp
@@ -383,6 +383,23 @@ AttributeStandard Attribute::name_standard(const char *name)
   return ATTR_STD_NONE;
 }
 
+AttrKernelDataType Attribute::kernel_type(const Attribute &attr)
+{
+  if (attr.element == ATTR_ELEMENT_CORNER) {
+    return AttrKernelDataType::UCHAR4;
+  }
+
+  if (attr.type == TypeDesc::TypeFloat) {
+    return AttrKernelDataType::FLOAT;
+  }
+
+  if (attr.type == TypeFloat2) {
+    return AttrKernelDataType::FLOAT2;
+  }
+
+  return AttrKernelDataType::FLOAT3;
+}
+
 void Attribute::get_uv_tiles(Geometry *geom,
                              AttributePrimitive prim,
                              unordered_set<int> &tiles) const
@@ -417,7 +434,7 @@ void Attribute::get_uv_tiles(Geometry *geom,
 /* Attribute Set */
 
 AttributeSet::AttributeSet(Geometry *geometry, AttributePrimitive prim)
-    : geometry(geometry), prim(prim)
+    : modified_flag(~0u), geometry(geometry), prim(prim)
 {
 }
 
@@ -440,7 +457,7 @@ Attribute *AttributeSet::add(ustring name, TypeDesc type, AttributeElement eleme
 
   Attribute new_attr(name, type, element, geometry, prim);
   attributes.emplace_back(std::move(new_attr));
-  modified = true;
+  tag_modified(attributes.back());
   return &attributes.back();
 }
 
@@ -462,8 +479,7 @@ void AttributeSet::remove(ustring name)
 
     for (it = attributes.begin(); it != attributes.end(); it++) {
       if (&*it == attr) {
-        modified = true;
-        attributes.erase(it);
+        remove(it);
         return;
       }
     }
@@ -608,8 +624,7 @@ void AttributeSet::remove(AttributeStandard std)
 
     for (it = attributes.begin(); it != attributes.end(); it++) {
       if (&*it == attr) {
-        modified = true;
-        attributes.erase(it);
+        remove(it);
         return;
       }
     }
@@ -634,6 +649,12 @@ void AttributeSet::remove(Attribute *attribute)
   }
 }
 
+void AttributeSet::remove(list<Attribute>::iterator it)
+{
+  tag_modified(*it);
+  attributes.erase(it);
+}
+
 void AttributeSet::resize(bool reserve_only)
 {
   foreach (Attribute &attr, attributes) {
@@ -674,15 +695,13 @@ void AttributeSet::update(AttributeSet &&new_attributes)
   for (it = attributes.begin(); it != attributes.end();) {
     if (it->std != ATTR_STD_NONE) {
       if (new_attributes.find(it->std) == nullptr) {
-        modified = true;
-        attributes.erase(it++);
+        remove(it++);
         continue;
       }
     }
     else if (it->name != "") {
       if (new_attributes.find(it->name) == nullptr) {
-        modified = true;
-        attributes.erase(it++);
+        remove(it++);
         continue;
       }
     }
@@ -699,7 +718,27 @@ void AttributeSet::clear_modified()
   foreach (Attribute &attr, attributes) {
     attr.modified = false;
   }
-  modified = false;
+
+  modified_flag = 0;
+}
+
+void AttributeSet::tag_modified(const Attribute &attr)
+{
+  /* Some attributes are not stored in the various kernel attribute arrays
+   * (DeviceScene::attribute_*), so the modified flags are only set if the associated standard
+   * corresponds to an attribute which will be stored in the kernel's attribute arrays. */
+  const bool modifies_device_array = (attr.std != ATTR_STD_FACE_NORMAL &&
+                                      attr.std != ATTR_STD_VERTEX_NORMAL);
+
+  if (modifies_device_array) {
+    AttrKernelDataType kernel_type = Attribute::kernel_type(attr);
+    modified_flag |= (1u << kernel_type);
+  }
+}
+
+bool AttributeSet::modified(AttrKernelDataType kernel_type) const
+{
+  return (modified_flag & (1u << kernel_type)) != 0;
 }
 
 /* AttributeRequest */
diff --git a/intern/cycles/render/attribute.h b/intern/cycles/render/attribute.h
index 18c9e5ab83a..004c267cabc 100644
--- a/intern/cycles/render/attribute.h
+++ b/intern/cycles/render/attribute.h
@@ -39,6 +39,21 @@ class Hair;
 class Mesh;
 struct Transform;
 
+/* AttrKernelDataType.
+ *
+ * The data type of the device arrays storing the attribute's data. Those data types are different
+ * than the ones for attributes as some attribute types are stored in the same array, e.g. Point,
+ * Vector, and Transform are all stored as float3 in the kernel.
+ *
+ * The values of this enumeration are also used as flags to detect changes in AttributeSet. */
+
+enum AttrKernelDataType {
+  FLOAT = 0,
+  FLOAT2 = 1,
+  FLOAT3 = 2,
+  UCHAR4 = 3,
+};
+
 /* Attribute
  *
  * Arbitrary data layers on meshes.
@@ -167,6 +182,8 @@ class Attribute {
   static const char *standard_name(AttributeStandard std);
   static AttributeStandard name_standard(const char *name);
 
+  static AttrKernelDataType kernel_type(const Attribute &attr);
+
   void get_uv_tiles(Geometry *geom, AttributePrimitive prim, unordered_set<int> &tiles) const;
 };
 
@@ -175,11 +192,12 @@ class Attribute {
  * Set of attributes on a mesh. */
 
 class AttributeSet {
+  uint32_t modified_flag;
+
  public:
   Geometry *geometry;
   AttributePrimitive prim;
   list<Attribute> attributes;
-  bool modified = true;
 
   AttributeSet(Geometry *geometry, AttributePrimitive prim);
   AttributeSet(AttributeSet &&) = default;
@@ -197,6 +215,8 @@ class AttributeSet {
 
   void remove(Attribute *attribute);
 
+  void remove(list<Attribute>::iterator it);
+
   void resize(bool reserve_only = false);
   void clear(bool preserve_voxel_data = false);
 
@@ -204,7 +224,18 @@ class AttributeSet {
    * and remove any attribute not found on the new set from this. */
   void update(AttributeSet &&new_attributes);
 
+  /* Return whether the attributes of the given kernel_type are modified, where "modified" means
+   * that some attributes of the given type were added or removed from this AttributeSet. This does
+   * not mean that the data of the remaining attributes in this AttributeSet were also modified. To
+   * check this, use Attribute.modified. */
+  bool modified(AttrKernelDataType kernel_type) const;
+
   void clear_modified();
+
+ private:
+  /* Set the relevant modified flag for the attribute. Only attributes that are stored in device
+   * arrays will be considered for tagging this AttributeSet as modified. */
+  void tag_modified(const Attribute &attr);
 };
 
 /* AttributeRequest
diff --git a/intern/cycles/render/geometry.cpp b/intern/cycles/render/geometry.cpp
index 16fc36231b4..1c4b360750f 100644
--- a/intern/cycles/render/geometry.cpp
+++ b/intern/cycles/render/geometry.cpp
@@ -830,10 +830,13 @@ void GeometryManager::device_update_attributes(Device *device,
   dscene->attributes_float3.alloc(attr_float3_size);
   dscene->attributes_uchar4.alloc(attr_uchar4_size);
 
-  const bool copy_all_data = dscene->attributes_float.need_realloc() ||
-                             dscene->attributes_float2.need_realloc() ||
-                             dscene->attributes_float3.need_realloc() ||
-                             dscene->attributes_uchar4.need_realloc();
+  /* The order of those flags needs to match that of AttrKernelDataType. */
+  const bool attributes_need_realloc[4] = {
+      dscene->attributes_float.need_realloc(),
+      dscene->attributes_float2.need_realloc(),
+      dscene->attributes_float3.need_realloc(),
+      dscene->attributes_uchar4.need_realloc(),
+  };
 
   size_t attr_float_offset = 0;
   size_t attr_float2_offset = 0;
@@ -852,7 +855,7 @@ void GeometryManager::device_update_attributes(Device *device,
 
       if (attr) {
         /* force a copy if we need to reallocate all the data */
-        attr->modified |= copy_all_data;
+        attr->modified |= attributes_need_realloc[Attribute::kernel_type(*attr)];
       }
 
       update_attribute_element_offset(geom,
@@ -875,7 +878,7 @@ void GeometryManager::device_update_attributes(Device *device,
 
         if (subd_attr) {
           /* force a copy if we need to reallocate all the data */
-          subd_attr->modified |= copy_all_data;
+          subd_attr->modified |= attributes_need_realloc[Attribute::kernel_type(*subd_attr)];
         }
 
         update_attribute_element_offset(mesh,
@@ -906,6 +909,10 @@ void GeometryManager::device_update_attributes(Device *device,
     foreach (AttributeRequest &req, attributes.requests) {
       Attribute *attr = values.find(req);
 
+      if (attr) {
+        attr->modified |= attributes_need_realloc[Attribute::kernel_type(*attr)];
+      }
+
       update_attribute_element_offset(object->geometry,
                                       dscene->attributes_float,
                                       attr_float_offset,
@@ -941,10 +948,10 @@ void GeometryManager::device_update_attributes(Device *device,
   /* copy to device */
   progress.set_status("Updating Mesh", "Copying Attributes to device");
 
-  dscene->attributes_float.copy_to_device();
-  dscene->attributes_float2.copy_to_device();
-  dscene->attributes_float3.copy_to_device();
-  dscene->attributes_uchar4.copy_to_device();
+  dscene->attributes_float.copy_to_device_if_modified();
+  dscene->attributes_float2.copy_to_device_if_modified();
+  dscene->attributes_float3.copy_to_device_if_modified();
+  dscene->attributes_uchar4.copy_to_device_if_modified();
 
   if (progress.get_cancel())
     return;
@@ -1431,24 +1438,46 @@ static void update_device_flags_attribute(uint32_t &device_update_flags,
       continue;
     }
 
-    if (attr.element == ATTR_ELEMENT_CORNER) {
-      device_update_flags |= ATTR_UCHAR4_MODIFIED;
-    }
-    else if (attr.type == TypeDesc::TypeFloat) {
-      device_update_flags |= ATTR_FLOAT_MODIFIED;
-    }
-    else if (attr.type == TypeFloat2) {
-      device_update_flags |= ATTR_FLOAT2_MODIFIED;
-    }
-    else if (attr.type == TypeDesc::TypeMatrix) {
-      device_update_flags |= ATTR_FLOAT3_MODIFIED;
-    }
-    else if (attr.element != ATTR_ELEMENT_VOXEL) {
-      device_update_flags |= ATTR_FLOAT3_MODIFIED;
+    AttrKernelDataType kernel_type = Attribute::kernel_type(attr);
+
+    switch (kernel_type) {
+      case AttrKernelDataType::FLOAT: {
+        device_update_flags |= ATTR_FLOAT_MODIFIED;
+        break;
+      }
+      case AttrKernelDataType::FLOAT2: {
+        device_update_flags |= ATTR_FLOAT2_MODIFIED;
+        break;
+      }
+      case AttrKernelDataType::FLOAT3: {
+        device_update_flags |= ATTR_FLOAT3_MODIFIED;
+        break;
+      }
+      case AttrKernelDataType::UCHAR4: {
+        device_update_flags |= ATTR_UCHAR4_MODIFIED;
+        break;
+      }
     }
   }
 }
 
+static void update_attribute_realloc_flags(uint32_t &device_update_flags,
+                                           const AttributeSet &attributes)
+{
+  if (attributes.modified(AttrKernelDataType::FLOAT)) {
+    device_update_flags |= ATTR_FLOAT_NEEDS_REALLOC;
+  }
+  if (attributes.modified(AttrKernelDataType::FLOAT2)) {
+    device_update_flags |= ATTR_FLOAT2_NEEDS_REALLOC;
+  }
+  if (attributes.modified(AttrKernelDataType::FLOAT3)) {
+    device_update_flags |= ATTR_FLOAT3_NEEDS_REALLOC;
+  }
+  if (attributes.modified(AttrKernelDataType::UCHAR4)) {
+    device_update_flags |= ATTR_UCHAR4_NEEDS_REALLOC;
+  }
+}
+
 void GeometryManager::device_update_preprocess(Device *device, Scene *scene, Progress &progress)
 {
   if (!need_update() && !need_flags_update) {
@@ -1471,16 +1500,11 @@ void GeometryManager::device_update_preprocess(Device *device, Scene *scene, Pro
   foreach (Geometry *geom, scene->geometry) {
     geom->has_volume = false;
 
-    if (geom->attributes.modified) {
-      device_update_flags |= ATTRS_NEED_REALLOC;
-    }
+    update_attribute_realloc_flags(device_update_flags, geom->attributes);
 
     if (geom->is_mesh()) {
       Mesh *mesh = static_cast<Mesh *>(geom);
-
-      if (mesh->subd_attributes.modified) {
-        device_update_flags |= ATTRS_NEED_REALLOC;
-      }
+      update_attribute_realloc_flags(device_update_flags, mesh->subd_attributes);
     }
 
     foreach (Node *node, geom->get_used_shaders()) {