Merge pull request #2026 from Idclip/leaf_val_iters

Improved performance of LeafNode ValueIters

Author: Idclip

openvdb/openvdb/Platform.h

@@ -92,6 +92,29 @@
 #define OPENVDB_UNLIKELY(x) (x)
 #endif
 
+/// Macros for assume builtins. Note that we currently don't simply apply these
+/// in place of asserts (when asserts are disabled) - they should be only be
+/// applied with an assert once profiled
+#ifdef __has_cpp_attribute
+  #if __has_cpp_attribute(assume) >= 202207L
+    #define OPENVDB_ASSUME(...) [[assume(__VA_ARGS__)]]
+  #endif
+#endif
+#ifndef OPENVDB_ASSUME
+  #if defined(__clang__)
+    #define OPENVDB_ASSUME(...)  __builtin_assume(__VA_ARGS__);
+  #elif defined(_MSC_VER)
+    #define OPENVDB_ASSUME(...) __assume(__VA_ARGS__);
+  #elif defined(__GNUC__)
+    #if __GNUC__ >= 13
+      #define OPENVDB_ASSUME(...) __attribute__((__assume__(__VA_ARGS__)))
+    #endif
+  #endif
+#endif
+#ifndef OPENVDB_ASSUME
+  #define OPENVDB_ASSUME(...)
+#endif
+
 /// Force inline function macros. These macros do not necessary guarantee that
 /// the decorated function will be inlined, but provide the strongest vendor
 /// annotations to that end.

openvdb/openvdb/tree/LeafNode.h

@@ -219,31 +219,60 @@ class LeafNode
         public SparseIteratorBase<
             MaskIterT, ValueIter<MaskIterT, NodeT, ValueT, TagT>, NodeT, ValueT>
     {
+        using ValueType = std::conditional_t<std::is_const_v<NodeT>, ValueT,
+            std::remove_const_t<ValueT>>;
         using BaseT = SparseIteratorBase<MaskIterT, ValueIter, NodeT, ValueT>;
 
         ValueIter() {}
-        ValueIter(const MaskIterT& iter, NodeT* parent): BaseT(iter, parent) {}
+        ValueIter(const MaskIterT& iter, NodeT* parent)
+            : BaseT(iter, parent)
+            // Unlike other value iterators, cache the buffer data as part of
+            // the iterators members to avoid the cost of going through the
+            // leaf buffer atomic/checking API
+            , mData([&]() { OPENVDB_ASSERT(parent); return parent->buffer().data(); }()) {}
 
-        ValueT& getItem(Index pos) const { return this->parent().getValue(pos); }
-        ValueT& getValue() const { return this->parent().getValue(this->pos()); }
+        ValueT& getItem(Index pos) const { return mData[pos]; }
+        ValueT& getValue() const { return this->getItem(this->pos()); }
 
         // Note: setItem() can't be called on const iterators.
         void setItem(Index pos, const ValueT& value) const
         {
-            this->parent().setValueOnly(pos, value);
+            if constexpr (std::is_const_v<NodeT>) {
+                static_assert(!std::is_const_v<NodeT>,
+                    "ValueIter::setItem cannot be called on const iterators");
+            }
+            else {
+                OPENVDB_ASSERT(pos < SIZE);
+                OPENVDB_ASSUME(pos < SIZE);
+                mData[pos] = value;
+            }
         }
         // Note: setValue() can't be called on const iterators.
-        void setValue(const ValueT& value) const
-        {
-            this->parent().setValueOnly(this->pos(), value);
-        }
+        void setValue(const ValueT& value) const { this->setItem(this->pos(), value); }
 
         // Note: modifyItem() can't be called on const iterators.
         template<typename ModifyOp>
-        void modifyItem(Index n, const ModifyOp& op) const { this->parent().modifyValue(n, op); }
+        void modifyItem(Index n, const ModifyOp& op) const
+        {
+            if constexpr (std::is_const_v<NodeT>) {
+                static_assert(!std::is_const_v<NodeT>,
+                    "ValueIter::modifyItem cannot be called on const iterators");
+            }
+            else {
+                OPENVDB_ASSERT(n < SIZE);
+                OPENVDB_ASSUME(n < SIZE);
+                op(mData[n]);
+                this->parent().setValueOn(n);
+            }
+        }
         // Note: modifyValue() can't be called on const iterators.
         template<typename ModifyOp>
-        void modifyValue(const ModifyOp& op) const { this->parent().modifyValue(this->pos(), op); }
+        void modifyValue(const ModifyOp& op) const
+        {
+            this->modifyItem(this->pos(), op);
+        }
+    private:
+        ValueType* mData;
     };
 
     /// Leaf nodes have no children, so their child iterators have no get/set accessors.

pendingchanges/leaf_value_iters.txt

@@ -0,0 +1,8 @@
+OpenVDB:
+ - Improvements:
+   - Significantly improved the performance of all LeafNode ValueIterators,
+     up to 5x on some platforms and up to 10x when delay loading is enabled.
+     Construction of a ValueIterator from a leaf node now requests the leaf
+     buffers ahead of iteration to avoid potentially expensive API calls.
+   - Added OPENVDB_ASSUME macros to mimic builtin assume and C++23 assume
+     attributes.