Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize SWAP macro by using move semantics. #100367

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 13, 2024

Move semantics help make code faster by changing ownership rather than copying objects. In the case of SWAP, a maximum of 3 copies can be avoided, if the underlying objects have corresponding constructors and assignment operators. The current implementation forces the compiler to copy the object for each assignment:

template <typename T>
 inline void __swap_tmpl(T &x, T &y) {
 	T aux = x;  // copy assignment
 	x = y;  // copy assignment
 	y = aux;  // copy assignment
 }

Recently, we starting to introduce move semantics into the codebase. In particular, there was #100239 for String, Vector and CowData. Hopefully, sometime in the future, we'll get similar move constructors and assignments for Variant. The SWAP macro should be ready to accelerate these semantics.

The implementation of std::swap on my machine (macOS) looks like this:

template <class _Tp>
inline _LIBCPP_HIDE_FROM_ABI __swap_result_t<_Tp> _LIBCPP_CONSTEXPR_SINCE_CXX20 swap(_Tp& __x, _Tp& __y)
    _NOEXCEPT_(is_nothrow_move_constructible<_Tp>::value&& is_nothrow_move_assignable<_Tp>::value) {
  _Tp __t(std::move(__x));
  __x = std::move(__y);
  __y = std::move(__t);
}

It could be trivially re-implemented in Godot as such:

template <typename T>
inline void __swap_tmpl(T &x, T &y) {
	T aux(std::move(x));
	x = std::move(y);
	y = std::move(aux);
}

However, since both std::swap and std::move are defined in <utility> (i.e. the include is needed anyway), I see no reason to reinvent the wheel (except maybe to make it constexpr?).

@Ivorforce Ivorforce requested a review from a team as a code owner December 13, 2024 15:23
@Chaosus Chaosus added this to the 4.4 milestone Dec 13, 2024
hpvb

This comment was marked as duplicate.

Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if we start seeing very strange compiler errors when this is misused we could revert to the "manual version" but I don't think that is likely in this case. std::swap() seems like a reasonable choice here.

@akien-mga akien-mga merged commit 68559c4 into godotengine:master Dec 14, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants