TraversableBase intrusive interface #288

DoeringChristian · 2024-09-25T13:29:17Z

This PR adds a TraversableBase class with the traverse_1_cb_ro and traverse_1_cb_rw functions and implements the nanobind::intrusive_base class.
The class can be implemented for any sub-class that should be traversable, and the functions can be generated by the DR_TRAVERSE macros.
It also changes the TraverseCallback::operator() function, which now returns a uint64_t. This is required to implement a make_opaque function, that is able to traverse C++ objects.

DoeringChristian · 2024-12-05T19:38:43Z

@merlinND, @wjakob This PR should be ready for review.
Note, there is one change from what we discussed last Friday.
I had to add a boolean to the argument of the traverse function, indicating whether traverse_cb_ro or traverse_cb_rw should be called. We cannot use the output of the callback to determine if the variable index should be overwritten, since overwriting it with $0$ might be desired (there is a test that fails in that case).

merlinND

Hi @DoeringChristian,

Thanks for this concise PR! The code looks good to me overall, but I'm not so familiar with this part of the codebase so @wjakob will probably need to take a look as well.

merlinND · 2024-12-12T10:43:24Z

include/drjit/array_traverse.h

+    } else if constexpr (is_detected_v<detail::det_traverse_1_cb_ro, Value *>) {
+        value.traverse_1_cb_ro(payload, fn);
+    } else {
+        // static_assert(false, "Failed to traverse field!");


Should the user be warned about this, or it's always fine to ignore?
In the latter case, you could remove the else block and add a comment to say explicitly that we silently return if we don't know how to traverse.

merlinND · 2024-12-12T10:43:47Z

include/drjit/array_traverse.h

@@ -15,6 +15,7 @@

 #pragma once

+#include <type_traits>


Suggested change

#include <type_traits>

#include <type_traits>

merlinND · 2024-12-12T10:45:21Z

include/drjit/python.h

@@ -54,6 +54,9 @@
 #include <drjit/math.h>
 #include <drjit-core/python.h>
 #include <nanobind/stl/array.h>
+#include <nanobind/intrusive/counter.h>
+#include "nanobind/nanobind.h"
+#include "drjit/traversable_base.h"


Sort the imports

merlinND · 2024-12-12T10:47:19Z

include/drjit/texture.h

@@ -41,7 +42,7 @@ enum class CudaTextureFormat : uint32_t {
    Float16 = 1, /// Half precision storage format
 };

-template <typename _Storage, size_t Dimension> class Texture {
+template <typename _Storage, size_t Dimension> class Texture : TraversableBase {


Is there no other DrJit class that needed to be made traversable?

merlinND · 2024-12-12T10:47:34Z

include/drjit/array_traverse.h

+    } else if constexpr (is_detected_v<detail::det_traverse_1_cb_rw, Value *>) {
+        value.traverse_1_cb_rw(payload, fn);
+    } else {
+        // static_assert(false, "Failed to traverse field!");


Same as above

merlinND · 2024-12-12T10:56:51Z

include/drjit/traversable_base.h

+    }
+
+/// Macro to generate traverse_1_cb_ro and traverse_1_cb_rw methods for each
+/// member in the list.


Expand the docstring to explain to users what they should do with this, which fields should be included or now, etc.
Add a warning to tell them what can happen if they fail to include a member in the list.

Also this information should be repeated in the frozen functions documentation, since it's a dangerous pitfall for users who have custom plugins / classes implemented.

merlinND · 2024-12-12T11:05:18Z

include/drjit/traversable_base.h

+#if defined(_MSC_VER)
+#define DRJIT_EXPORT __declspec(dllexport)
+#else
+#define DRJIT_EXPORT __attribute__((visibility("default")))
+#endif


Isn't this already defined somewhere else?

merlinND · 2024-12-12T11:06:28Z

src/python/apply.h

+extern void traverse(const char *op, TraverseCallback &callback, nb::handle h,
+                     bool rw = false);


Document the rw parameter, explain why it's needed and what the user should pass.

merlinND · 2024-12-12T11:06:44Z

src/python/apply.h

@@ -57,7 +57,7 @@ struct TraverseCallback {
    // Type-erased form which is needed in some cases to traverse into opaque
    // C++ code. This one just gets called with Jit/AD variable indices, an
    // associated Python/ instance/type is not available.
-    virtual void operator()(uint64_t index);
+    virtual uint64_t operator()(uint64_t index);


Document the return value.

merlinND · 2024-12-12T11:08:43Z

src/python/apply.cpp

                cb(h, nb::cpp_function([&](uint64_t index) { tc(index); }));
+            } else if (nb::object cb = get_traverse_cb_rw(tp); cb.is_valid() && rw) {
+                cb(h, nb::cpp_function([&](uint64_t index) { return tc(index); }));


If the user passed rw = true but only get_traverse_cb_ro(tp) is valid, not get_traverse_cb_rw(tp), is that an error? Should we throw?

DoeringChristian force-pushed the traversable-base branch from 29392b0 to 33ed806 Compare September 25, 2024 13:34

DoeringChristian marked this pull request as draft September 27, 2024 08:44

DoeringChristian force-pushed the traversable-base branch 2 times, most recently from f2a0e24 to a4632c1 Compare September 27, 2024 10:59

DoeringChristian force-pushed the traversable-base branch 2 times, most recently from b0a1d57 to 5293619 Compare October 18, 2024 09:11

DoeringChristian force-pushed the traversable-base branch from 5293619 to 33dfe2a Compare October 21, 2024 14:16

DoeringChristian force-pushed the traversable-base branch 2 times, most recently from 31fb70f to ca2680d Compare December 2, 2024 10:25

DoeringChristian added 6 commits December 2, 2024 11:26

Added TraversableBase class for c++ type traversal

798740b

Improved traversal of c++ iterators and references

00857c1

Using TraversableBase for Texture traversal

5038256

Added traversal helpers for trampoline classes

fa974fa

Fixed double definition of traverse_py_cb_* methods

d0fc270

Changed traverse function, allowing assigment of c++ objects

4efa918

DoeringChristian force-pushed the traversable-base branch from ca2680d to 4cc7be1 Compare December 2, 2024 10:26

Added traversal tests

4a7a773

DoeringChristian force-pushed the traversable-base branch from 4cc7be1 to 4a7a773 Compare December 2, 2024 14:42

DoeringChristian mentioned this pull request Dec 5, 2024

Frozen Functions #317

Draft

DoeringChristian marked this pull request as ready for review December 5, 2024 19:35

merlinND reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TraversableBase intrusive interface #288

TraversableBase intrusive interface #288

DoeringChristian commented Sep 25, 2024 •

edited

Loading

DoeringChristian commented Dec 5, 2024

merlinND left a comment

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

merlinND Dec 12, 2024

		extern void traverse(const char *op, TraverseCallback &callback, nb::handle h,
		bool rw = false);

TraversableBase intrusive interface #288

Are you sure you want to change the base?

TraversableBase intrusive interface #288

Conversation

DoeringChristian commented Sep 25, 2024 • edited Loading

DoeringChristian commented Dec 5, 2024

merlinND left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoeringChristian commented Sep 25, 2024 •

edited

Loading