[smart_holder] Bake smart_holder functionality into `class_` and `type_caster_base` #5257

rwgk · 2024-07-21T03:09:30Z

Description

This PR is a continuation of PR #5213 on top of the smart_holder branch.

Overarching goal: Prepare the smart_holder branch for merging into master, to start the pybind11 v3 line.

Note: This PR restores the existing unit tests on the master branch, with very small exceptions. See PR #5213 Description for details.

High-level trade-off:

smart_holder_type_casters (smart_holder branch before this PR) were an add-on and the PYBIND11_INTERNALS_VERSION did not have to be changed.
For the baked-in smart_holder support (smart_holder branch after this PR), the PYBIND11_INTERNALS_VERSION needs to be changed, but there is far fewer duplicated code.

Major benefit: The PYBIND11_SMART_HOLDER_TYPE_CASTERS macro is obsolete (it still exists for backward compatibility but is a no-op).

Added benefit: Testing with smart_holder as the default holder is far less important. Testing with smart_holder as the default holder will be cut back (in a follow-on PR) to one each for Linux, macOS, Windows.

NOTE: The baked-in smart_holder functionality can easily be disabled by overriding the PYBIND11_INTERNALS_VERSION: setting it to 5 (or even 4) almost completely #ifdefs out all code added in this PR.

Work for follow-on PRs: Resolve all SMART_HOLDER_BAKEIN_FOLLOW_ON. This work is intentionally deferred because it will change existing code on the master branch in non-trivial ways.

Suggested order for code review: (internal CL)

include/pybind11/detail/internals.h — NEW enum class holder_enum_t.
include/pybind11/cast.h — NEW copyable_holder_caster and move_only_holder_caster specializations for std::shared_ptr and std::unique_ptr.
include/pybind11/detail/type_caster_base.h — smart_holder_from_unique_ptr and smart_holder_from_shared_ptr implementations (based on code originally in smart_holder_type_casters.h).
include/pybind11/pybind11.h — NEW init_instance() specialization (based on code originally in smart_holder_type_casters.h) and modified property_cpp_function specializations.
Everything else under include/pybind11.

Before reviewing the test changes, please read the description of PR #5213. Most importantly, almost all changes to tests on the master branch are undone. The test_class_sh_*.cpp,py changes in this PR are essentially pure boilerplate changes or trivial.

Suggested changelog entry:

Commands used: ``` git checkout bakein git diff smart_holder > ~/zd git checkout smart_holder git checkout -b bakein_sh patch -p 1 < ~/zd git checkout smart_holder \ MANIFEST.in \ README.rst \ README_smart_holder.rst \ docs/advanced/smart_ptrs.rst \ ubench/holder_comparison.cpp \ ubench/holder_comparison.py \ ubench/holder_comparison_extract_sheet_data.py \ ubench/number_bucket.h \ ubench/python/number_bucket.clif git add -A ```

…iles.py

laramiel · 2024-07-22T22:41:41Z

include/pybind11/detail/smart_holder_poc.h

    std::shared_ptr<void> vptr;
+    PYBIND11_SMART_HOLDER_PADDING(3);


padding between bool bitfields? Why use bitfields in any case?

The padding was for brute-force stress-testing only while working on #5213. I just reverted it here (c02d2cd). It's easy enough to redo if/as needed.

(The bitfields are just to keep the memory footprint small.)

…YBIND11_SMART_HOLDER_PADDING`, which was meant for stress-testing only).

laramiel · 2024-07-23T16:06:18Z

include/pybind11/pybind11.h

-                            is_method(hdl));
+        detail::type_info *tinfo = detail::get_type_info(typeid(T), /*throw_if_missing=*/true);
+        if (tinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
+            return cpp_function([pm](T &c, D value) { c.*pm = std::forward<D>(value); },


Done: 99b6572

Thanks for catching this!

laramiel · 2024-07-23T16:16:52Z

include/pybind11/pybind11.h

+PYBIND11_NAMESPACE_END(detail)
+
+#ifdef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+


You could do this here:

template <typename T, typename D> struct property_cpp_function__version6_pointer_something { template <typename PM, detail::must_be_member_function_pointer<PM> = 0> static cpp_function readonly(PM pm, const handle &hdl) { detail::type_info *tinfo = detail::get_type_info(typeid(T), /*throw_if_missing=*/true); if (tinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { return cpp_function( [pm](handle c_hdl) -> std::shared_ptr<drp> { std::shared_ptr<T> c_sp = detail::type_caster< std::shared_ptr<T>>::shared_ptr_with_responsible_parent(c_hdl); D ptr = (*c_sp).*pm; return std::shared_ptr<drp>(c_sp, ptr); }, is_method(hdl)); } return property_cpp_function<T, D, void>::readonly(std::move(pm), hdl); } template <typename PM, detail::must_be_member_function_pointer<PM> = 0> static cpp_function read(PM pm, const handle &hdl) { return readonly(pm, hdl); } template <typename PM, detail::must_be_member_function_pointer<PM> = 0> static cpp_function write(PM pm, const handle &hdl) { detail::type_info *tinfo = detail::get_type_info(typeid(T), /*throw_if_missing=*/true); if (tinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { return cpp_function([pm](T &c, D value) { c.*pm = std::move<D>(value); }, is_method(hdl)); } return property_cpp_function<T, D, void>::write(std::move(pm), hdl); } }; template <typename T, typename D> struct property_cpp_function<T, D, detail::enable_if_t<detail::all_of< detail::none_of<std::is_pointer<D>, std::is_array<D>, detail::is_instantiation<std::unique_ptr, D>, detail::is_instantiation<std::shared_ptr, D>>, detail::both_t_and_d_use_type_caster_base<T, D>>::value>> : public property_cpp_function__version6_pointer_something {};

Done: 16cf7ad

That is surely easier to read and nicely enabled this follow-on commit: 0bcfbd4

laramiel · 2024-07-23T16:32:31Z

include/pybind11/trampoline_self_life_support.h

+
+#ifdef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+
+#    include "detail/common.h"


Do these need to be ifdef guarded?

It's a choice (for the most part at least).

I did it that way so that it is as obvious as possible that pybind11 behaves almost exactly as before when PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT is not defined.

(The few tiny exceptions are a compromise, #ifdef clutter vs. tiny pieces from the smart_holder code leaking in.)

I prefer to keep this as-is, unless you believe it's better to not have conditional #includes.

laramiel · 2024-07-23T22:31:06Z

include/pybind11/detail/type_caster_base.h

+                                    return_value_policy policy,
+                                    handle parent,
+                                    const std::pair<const void *, const type_info *> &st) {
+    switch (policy) {


Why use a switch here? To catch policy enum changes, or what? Why not use a switch above?

To catch policy enum changes

Yes, that exactly was the idea (similar to this existing code on master):

pybind11/include/pybind11/detail/type_caster_base.h

Lines 506 to 566 in 8e7307f

switch (policy) {

case return_value_policy::automatic:

case return_value_policy::take_ownership:

valueptr = src;

wrapper->owned = true;

break;

case return_value_policy::automatic_reference:

case return_value_policy::reference:

valueptr = src;

wrapper->owned = false;

break;

case return_value_policy::copy:

if (copy_constructor) {

valueptr = copy_constructor(src);

} else {

#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)

std::string type_name(tinfo->cpptype->name());

detail::clean_type_id(type_name);

throw cast_error("return_value_policy = copy, but type " + type_name

+ " is non-copyable!");

#else

throw cast_error("return_value_policy = copy, but type is "

"non-copyable! (#define PYBIND11_DETAILED_ERROR_MESSAGES or "

"compile in debug mode for details)");

#endif

}

wrapper->owned = true;

break;

case return_value_policy::move:

if (move_constructor) {

valueptr = move_constructor(src);

} else if (copy_constructor) {

valueptr = copy_constructor(src);

} else {

#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)

std::string type_name(tinfo->cpptype->name());

detail::clean_type_id(type_name);

throw cast_error("return_value_policy = move, but type " + type_name

+ " is neither movable nor copyable!");

#else

throw cast_error("return_value_policy = move, but type is neither "

"movable nor copyable! "

"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in "

"debug mode for details)");

#endif

}

wrapper->owned = true;

break;

case return_value_policy::reference_internal:

valueptr = src;

wrapper->owned = false;

keep_alive_impl(inst, parent);

break;

default:

throw cast_error("unhandled return_value_policy: should not happen!");

}

Ideally we'd have more like comprehensive switch like that, to guard against accidents and misunderstandings. (I made a partial pass in the past, but the gain isn't that big, and there were always bigger fish to fry...)

laramiel · 2024-07-23T22:32:11Z

include/pybind11/detail/type_caster_base.h

+                                    return_value_policy policy,
+                                    handle parent,
+                                    const std::pair<const void *, const type_info *> &st) {
+    if (policy != return_value_policy::automatic


Positive tests are easier to follow. This function is only valid for return_value_policy::copy

Done: cb6a85f

The list of conditions started small and grew over time (to keep existing client code happy). TBH I didn't fully realize that only copy is invalid now.

laramiel · 2024-07-23T22:34:54Z

include/pybind11/detail/type_caster_base.h

+        return std::shared_ptr<T>(raw_ptr, shared_ptr_parent_life_support(parent.ptr()));
+    }
+
+    std::shared_ptr<T> loaded_as_shared_ptr(void *void_raw_ptr,


nit. naming. loaded_as vs. load_as?

…laramiel

…laramiel (pybind#5257 (comment))

…lining the implementations.

…parison.cpp (holder_comparison.py is NOT changed accordingly in this commit, i.e. can still only be run if the smart_holder functionality is available).

@laramiel

…_ptr`) as suggested by @laramiel

…s that the latest implementation of `smart_holder_from_unique_ptr()` accepts all existing `return_value_policy` enum values except `copy`.

…specializations.

…-testing only.`

Leave only two pairs of SMART_HOLDER_BAKEIN_FOLLOW_ON comments: refactoring of copyable_holder_caster, move_only_holder_caster. This is best left until after the smart_holder branch is merged into the master branch.

…der`

laramiel · 2024-07-30T16:45:23Z

include/pybind11/numpy.h

@@ -1986,7 +1990,7 @@ struct vectorize_helper {
        // Pointers to values the function was called with; the vectorized ones set here will start
        // out as array_t<T> pointers, but they will be changed them to T pointers before we make
        // call the wrapped function.  Non-vectorized pointers are left as-is.
-        std::array<void *, N> params{{&args...}};
+        std::array<void *, N> params{{reinterpret_cast<void *>(&args)...}};


It seems like most uses of reinterpret_cast<void*>(&...) in this cl should be changed to static_cast<void*>(&...) instead.

Oh, sorry, these came in through PR #5272 yesterday, which was merged already on master. — I'm not using rebase and push --force here (or anywhere really) because that invalidates a lot of commit hashes, and then I have no way to easily refer to commits from comments.

The best way to review here is to start with the "Files changed" tab near the top of this PR.

While working on PR #5272, I tried static_cast<void *> and it built successfully, but clang-tidy (clang version 18) still complained. reinterpret_cast was to only way to make it happy. — I didn't try to understand exactly why. — I was thinking the reinterpret_casts are better than // NOLINTs.

The best way to review here is to start with the "Files changed" tab near the top of this PR.

Oh, sorry, I just now realized it doesn't look the way I was expecting. Ugh!

Here is a helper PR showing the net diff (what I was wrongly hoping "Files changed" would show):

https://github.com/pybind/pybind11/pull/5277/files

Ok. So perhaps make this change at head. :)

laramiel · 2024-07-31T08:38:43Z

This looks reasonable to me and good to merge.

rwgk · 2024-07-31T13:17:21Z

Thanks a ton for the review @laramiel!

…ializations. The * `copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled` and * `move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled` SFINAE helpers were introduced with pybind/pybind11#5257. They need to be specialized here (`std::false_type`) because native_proto_caster.h has its own specializations for * `copyable_holder_caster<ProtoType, std::shared_ptr<ProtoType>>` and * `move_only_holder_caster<ProtoType, std::unique_ptr<ProtoType>>`. PiperOrigin-RevId: 658807442

…ializations. The * `copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled` and * `move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled` SFINAE helpers were introduced with pybind/pybind11#5257. They need to be specialized here (`std::false_type`) because native_proto_caster.h has its own specializations for * `copyable_holder_caster<ProtoType, std::shared_ptr<ProtoType>>` and * `move_only_holder_caster<ProtoType, std::unique_ptr<ProtoType>>`. PiperOrigin-RevId: 658867114

Ralf W. Grosse-Kunstleve added 2 commits July 20, 2024 20:04

Add back README_smart_holder.rst in tests/extra_python_package/test_f…

fbd1295

…iles.py

rwgk mentioned this pull request Jul 22, 2024

[DEV-master-smart_holder] Bake smart_holder functionality into class_ and type_caster_base #5213

Closed

Merge branch 'smart_holder' into bakein_sh

58f9e18

laramiel reviewed Jul 22, 2024

View reviewed changes

Restore smart_holder_poc.h as-is on smart_holder branch (i.e. undo `P…

c02d2cd

…YBIND11_SMART_HOLDER_PADDING`, which was meant for stress-testing only).

laramiel reviewed Jul 23, 2024

View reviewed changes

Ralf W. Grosse-Kunstleve added 12 commits July 25, 2024 21:25

Insert std::move() as suggested by @laramiel

99b6572

property_cpp_function_sh_* named specializations, as suggested by @…

16cf7ad

…laramiel (pybind#5257 (comment))

Call property_cpp_function_classic member functions, rather than in…

0bcfbd4

…lining the implementations.

Use PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT in holder_com…

2644d6e

…parison.cpp (holder_comparison.py is NOT changed accordingly in this commit, i.e. can still only be run if the smart_holder functionality is available).

Systematically rename loaded_as to load_as (shared_ptr, `unique…

336860d

…_ptr`) as suggested by @laramiel

Make change as suggested by @laramiel. This makes it much more obviou…

cb6a85f

…s that the latest implementation of `smart_holder_from_unique_ptr()` accepts all existing `return_value_policy` enum values except `copy`.

Resolve BAKEIN_WIP: Rewrite comment. for property_cpp_function_* …

0316f74

…specializations.

Resolve `BAKEIN_WIP: Add comment to explain: This is meant for stress…

053467f

…-testing only.`

Resolve all remaining BAKEIN_WIP (in pybind11/cast.h).

523eafa

Leave only two pairs of SMART_HOLDER_BAKEIN_FOLLOW_ON comments: refactoring of copyable_holder_caster, move_only_holder_caster. This is best left until after the smart_holder branch is merged into the master branch.

Remove obsolete using holder_type = smart_holder; in load_helper

7c0aed7

Add SMART_HOLDER_BAKEIN_FOLLOW_ON comment for `internals::default_hol…

0ca3ca7

…der`

README_smart_holder.rst update (line count reduced from 356 to 123).

4a7f895

rwgk marked this pull request as ready for review July 27, 2024 08:37

rwgk requested a review from henryiii as a code owner July 27, 2024 08:37

Ralf W. Grosse-Kunstleve added 2 commits July 29, 2024 11:25

Merge branch 'sh_merge_master' into bakein_sh_merge_ahead

67ac541

Merge branch 'sh_merge_master' into bakein_sh

8ffe837

rwgk changed the title ~~[smart_holder][WIP] Put bakein branch on top of smart_holder branch.~~ [smart_holder] Bake smart_holder functionality into class_ and type_caster_base Jul 30, 2024

laramiel reviewed Jul 30, 2024

View reviewed changes

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 30, 2024

PR pybind#5257 latest but fresh on smart_holder (as one commit)

7a6d96a

Merge branch 'smart_holder' into bakein_sh

2e9769a

rwgk merged commit 48f2527 into pybind:smart_holder Jul 31, 2024
138 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 31, 2024

rwgk deleted the bakein_sh branch July 31, 2024 13:17

rwgk removed the needs changelog Possibly needs a changelog entry label Jul 31, 2024

rwgk mentioned this pull request Aug 1, 2024

Remove include of smart_holder_type_casters.h header google-deepmind/open_spiel#1258

Merged

copybara-service bot mentioned this pull request Aug 2, 2024

native_proto_caster.h: Add *_with_smart_holder_support_enabled specializations. pybind/pybind11_protobuf#170

Merged

rwgk mentioned this pull request Aug 26, 2024

[smart_holder] Remove test_classh_mock.cpp,py #5338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[smart_holder] Bake smart_holder functionality into `class_` and `type_caster_base` #5257

[smart_holder] Bake smart_holder functionality into `class_` and `type_caster_base` #5257

rwgk commented Jul 21, 2024 •

edited

Loading

laramiel Jul 22, 2024

rwgk Jul 23, 2024

laramiel Jul 23, 2024

rwgk Jul 26, 2024

laramiel Jul 23, 2024 •

edited

Loading

rwgk Jul 26, 2024

laramiel Jul 23, 2024

rwgk Jul 26, 2024

laramiel Jul 23, 2024

rwgk Jul 26, 2024

laramiel Jul 23, 2024

rwgk Jul 26, 2024

laramiel Jul 23, 2024

laramiel Jul 30, 2024

rwgk Jul 30, 2024

rwgk Jul 30, 2024

rwgk Jul 30, 2024

laramiel Jul 31, 2024

laramiel commented Jul 31, 2024

rwgk commented Jul 31, 2024

		std::shared_ptr<void> vptr;
		PYBIND11_SMART_HOLDER_PADDING(3);

		PYBIND11_NAMESPACE_END(detail)

		#ifdef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT


		#ifdef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT

		# include "detail/common.h"

	switch (policy) {
	case return_value_policy::automatic:
	case return_value_policy::take_ownership:
	valueptr = src;
	wrapper->owned = true;
	break;

	case return_value_policy::automatic_reference:
	case return_value_policy::reference:
	valueptr = src;
	wrapper->owned = false;
	break;

	case return_value_policy::copy:
	if (copy_constructor) {
	valueptr = copy_constructor(src);
	} else {
	#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
	std::string type_name(tinfo->cpptype->name());
	detail::clean_type_id(type_name);
	throw cast_error("return_value_policy = copy, but type " + type_name
	+ " is non-copyable!");
	#else
	throw cast_error("return_value_policy = copy, but type is "
	"non-copyable! (#define PYBIND11_DETAILED_ERROR_MESSAGES or "
	"compile in debug mode for details)");
	#endif
	}
	wrapper->owned = true;
	break;

	case return_value_policy::move:
	if (move_constructor) {
	valueptr = move_constructor(src);
	} else if (copy_constructor) {
	valueptr = copy_constructor(src);
	} else {
	#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
	std::string type_name(tinfo->cpptype->name());
	detail::clean_type_id(type_name);
	throw cast_error("return_value_policy = move, but type " + type_name
	+ " is neither movable nor copyable!");
	#else
	throw cast_error("return_value_policy = move, but type is neither "
	"movable nor copyable! "
	"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in "
	"debug mode for details)");
	#endif
	}
	wrapper->owned = true;
	break;

	case return_value_policy::reference_internal:
	valueptr = src;
	wrapper->owned = false;
	keep_alive_impl(inst, parent);
	break;

	default:
	throw cast_error("unhandled return_value_policy: should not happen!");
	}

[smart_holder] Bake smart_holder functionality into class_ and type_caster_base #5257

[smart_holder] Bake smart_holder functionality into class_ and type_caster_base #5257

Conversation

rwgk commented Jul 21, 2024 • edited Loading

Description

Suggested changelog entry:

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

laramiel Jul 23, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laramiel commented Jul 31, 2024

rwgk commented Jul 31, 2024

[smart_holder] Bake smart_holder functionality into `class_` and `type_caster_base` #5257

[smart_holder] Bake smart_holder functionality into `class_` and `type_caster_base` #5257

rwgk commented Jul 21, 2024 •

edited

Loading

laramiel Jul 23, 2024 •

edited

Loading