Optimize `RequiredParam` to not increase and decrease refcounts on call.

core/variant/method_ptrcall.h

@@ -273,7 +273,7 @@ struct PtrToArg<const T *> {
 
 template <class T>
 struct PtrToArg<RequiredParam<T>> {
-	typedef typename RequiredParam<T>::ptr_type EncodeT;
+	typedef typename RequiredParam<T>::persistent_type EncodeT;
 
 	_FORCE_INLINE_ static RequiredParam<T> convert(const void *p_ptr) {
 		if (p_ptr == nullptr) {
@@ -283,7 +283,7 @@ struct PtrToArg<RequiredParam<T>> {
 	}
 
 	_FORCE_INLINE_ static void encode(const RequiredParam<T> &p_var, void *p_ptr) {
-		*((typename RequiredParam<T>::ptr_type *)p_ptr) = p_var._internal_ptr_dont_use();
+		*((typename RequiredParam<T>::persistent_type *)p_ptr) = p_var._internal_ptr_dont_use();
 	}
 };
 

core/variant/required_ptr.h

@@ -153,24 +153,31 @@ class RequiredParam {
 	static_assert(!is_fully_defined_v<T> || std::is_base_of_v<Object, T>, "T must be an Object subtype");
 
 public:
+	static constexpr bool is_ref = std::is_base_of_v<RefCounted, T>;
+
 	using element_type = T;
-	using ptr_type = std::conditional_t<std::is_base_of_v<RefCounted, T>, Ref<T>, T *>;
+	using extracted_type = std::conditional_t<is_ref, const Ref<T> &, T *>;
+	using persistent_type = std::conditional_t<is_ref, Ref<T>, T *>;
 
 private:
-	ptr_type _value = ptr_type();
+	T *_value = nullptr;
 
 	_FORCE_INLINE_ RequiredParam() = default;
 
 public:
 	// These functions should not be called directly, they are only for internal use.
-	_FORCE_INLINE_ ptr_type _internal_ptr_dont_use() const { return _value; }
-	_FORCE_INLINE_ bool _is_null_dont_use() const {
-		if constexpr (std::is_base_of_v<RefCounted, T>) {
-			return _value.is_null();
+	_FORCE_INLINE_ extracted_type _internal_ptr_dont_use() const {
+		if constexpr (is_ref) {
+			// Pretend _value is a Ref, for ease of use with existing `const Ref &` accepting APIs.
+			// This only works as long as Ref is internally T *.
+			// The double indirection should be optimized away by the compiler.
+			static_assert(sizeof(Ref<T>) == sizeof(T *));
+			return *((const Ref<T> *)&_value);
 		} else {
-			return _value == nullptr;
+			return _value;
 		}
 	}
+	_FORCE_INLINE_ bool _is_null_dont_use() const { return _value == nullptr; }
 	_FORCE_INLINE_ static RequiredParam<T> _err_return_dont_use() { return RequiredParam<T>(); }
 
 	// Prevent erroneously assigning null values by explicitly removing nullptr constructor/assignment.
@@ -202,22 +209,13 @@ public:
 
 	template <typename T_Other, std::enable_if_t<std::is_base_of_v<T, T_Other>, int> = 0>
 	_FORCE_INLINE_ RequiredParam(const Ref<T_Other> &p_ref) :
-			_value(p_ref) {}
+			_value(*p_ref) {}
 	template <typename T_Other, std::enable_if_t<std::is_base_of_v<T, T_Other>, int> = 0>
 	_FORCE_INLINE_ RequiredParam &operator=(const Ref<T_Other> &p_ref) {
-		_value = p_ref;
+		_value = *p_ref;
 		return *this;
 	}
 
-	template <typename T_Other, std::enable_if_t<std::is_base_of_v<T, T_Other>, int> = 0>
-	_FORCE_INLINE_ RequiredParam(Ref<T_Other> &&p_ref) :
-			_value(std::move(p_ref)) {}
-	template <typename T_Other, std::enable_if_t<std::is_base_of_v<T, T_Other>, int> = 0>
-	_FORCE_INLINE_ RequiredParam &operator=(Ref<T_Other> &&p_ref) {
-		_value = std::move(p_ref);
-		return &this;
-	}
-
 	template <typename U = T, std::enable_if_t<std::is_base_of_v<RefCounted, U>, int> = 0>
 	_FORCE_INLINE_ RequiredParam(const Variant &p_variant) :
 			_value(static_cast<T *>(p_variant.get_validated_object())) {}
@@ -242,7 +240,7 @@ public:
 		_err_print_error(FUNCTION_STR, __FILE__, __LINE__, "Required object \"" _STR(m_param) "\" is null.", m_msg, m_editor); \
 		return m_retval;                                                                                                       \
 	}                                                                                                                          \
-	typename std::decay_t<decltype(m_param)>::ptr_type m_name = m_param._internal_ptr_dont_use();                              \
+	typename std::decay_t<decltype(m_param)>::extracted_type m_name = m_param._internal_ptr_dont_use();                        \
 	static_assert(true)
 
 // These macros are equivalent to the ERR_FAIL_NULL*() family of macros, only for RequiredParam<T> instead of raw pointers.

tests/core/object/test_object.h

@@ -596,26 +596,27 @@ TEST_CASE("[Object] Destruction at the end of the call chain is safe") {
 			"Object was tail-deleted without crashes.");
 }
 
-int required_param_compare(const Ref<RefCounted> &p_ref, const RequiredParam<RefCounted> &p_required) {
-	EXTRACT_PARAM_OR_FAIL_V(extract, p_required, false);
-	ERR_FAIL_COND_V(p_ref->get_reference_count() != extract->get_reference_count(), -1);
+int required_param_compare(const Ref<RefCounted> &p_ref, const RequiredParam<RefCounted> &rp_required) {
+	EXTRACT_PARAM_OR_FAIL_V(p_required, rp_required, false);
+	ERR_FAIL_COND_V(p_ref->get_reference_count() != p_required->get_reference_count(), -1);
 	return p_ref->get_reference_count();
 }
 
 TEST_CASE("[Object] RequiredParam Ref<T>") {
 	Ref<RefCounted> ref;
 	ref.instantiate();
+	const Ref<RefCounted> &ref_ref = ref;
 
 	RequiredParam<RefCounted> required = ref;
 	EXTRACT_PARAM_OR_FAIL(extract, required);
 
-	static_assert(std::is_same_v<decltype(ref), decltype(extract)>);
+	static_assert(std::is_same_v<decltype(ref_ref), decltype(extract)>);
 
 	CHECK_EQ(ref->get_reference_count(), extract->get_reference_count());
 
 	const int count = required_param_compare(ref, ref);
 	CHECK_NE(count, -1);
-	CHECK_NE(count, ref->get_reference_count());
+	CHECK_EQ(count, ref->get_reference_count());
 
 	CHECK_EQ(ref->get_reference_count(), extract->get_reference_count());
 }

tests/scene/test_instance_placeholder.h

@@ -110,7 +110,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with no
 		scene->set_int_property(12);
 
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		const Error err = packed_scene->pack(scene);
 		REQUIRE(err == OK);
 
@@ -138,7 +138,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with no
 		referenced->set_owner(scene);
 		scene->set_reference_property(referenced);
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		const Error err = packed_scene->pack(scene);
 		REQUIRE(err == OK);
 
@@ -175,7 +175,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with no
 		node_array.push_back(referenced2);
 		scene->set_reference_array_property(node_array);
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		const Error err = packed_scene->pack(scene);
 		REQUIRE(err == OK);
 
@@ -219,7 +219,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with ov
 		scene->set_int_property(12);
 
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		packed_scene->pack(scene);
 
 		// Instantiate the scene.
@@ -248,7 +248,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with ov
 		referenced->set_owner(scene);
 		scene->set_reference_property(referenced);
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		const Error err = packed_scene->pack(scene);
 		REQUIRE(err == OK);
 
@@ -303,7 +303,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instantiate from placeholder with ov
 
 		scene->set_reference_array_property(referenced_array);
 		// Pack the scene.
-		PackedScene *packed_scene = memnew(PackedScene);
+		Ref<PackedScene> packed_scene = memnew(PackedScene);
 		const Error err = packed_scene->pack(scene);
 		REQUIRE(err == OK);
 
@@ -346,7 +346,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instance a PackedScene containing an
 	internal->set_reference_property(referenced);
 
 	// Pack the internal scene.
-	PackedScene *internal_scene = memnew(PackedScene);
+	Ref<PackedScene> internal_scene = memnew(PackedScene);
 	Error err = internal_scene->pack(internal);
 	REQUIRE(err == OK);
 
@@ -375,7 +375,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instance a PackedScene containing an
 	internal_created->set("reference_property", NodePath("OriginalReference"));
 
 	// Pack the main scene.
-	PackedScene *main_scene = memnew(PackedScene);
+	Ref<PackedScene> main_scene = memnew(PackedScene);
 	err = main_scene->pack(root);
 	REQUIRE(err == OK);
 
@@ -435,7 +435,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instance a PackedScene containing an
 	internal->set_reference_array_property(referenced_array);
 
 	// Pack the internal scene.
-	PackedScene *internal_scene = memnew(PackedScene);
+	Ref<PackedScene> internal_scene = memnew(PackedScene);
 	Error err = internal_scene->pack(internal);
 	REQUIRE(err == OK);
 
@@ -476,7 +476,7 @@ TEST_CASE("[SceneTree][InstancePlaceholder] Instance a PackedScene containing an
 	internal_created->set_reference_array_property(override_array);
 
 	// Pack the main scene.
-	PackedScene *main_scene = memnew(PackedScene);
+	Ref<PackedScene> main_scene = memnew(PackedScene);
 	err = main_scene->pack(root);
 	REQUIRE(err == OK);