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

Add move constructor and move assignment to CowData, String, Char16String, CharString, and Vector. #100239

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 10, 2024

This will allow the compiler to optimize transfer of data better. Previously, a transfer would necessitate incrementing and decrementing the refcount unnecessarily. Now, it is possible to simply transfer the data by swapping pointer ownership.

The new constructor and assignment operators are selected automatically, instead of the copy versions, for compatible callers, without any explicit adjustments required.

This should grease the gears and make some parts of Godot faster and / or smaller and / or compile faster by a nebulous small factor.

@Ivorforce Ivorforce requested a review from a team as a code owner December 10, 2024 12:44
@Ivorforce Ivorforce force-pushed the cowdata-move-constructor branch 2 times, most recently from 311b953 to ee8bb5e Compare December 10, 2024 12:54
@Ivorforce Ivorforce changed the title Add move constructor and move assignment to CowData, String, Char16String, CharString and Vector. Add move constructor and move assignment to CowData, String, Char16String, CharString, Vector and Variant. Dec 10, 2024
@Ivorforce Ivorforce force-pushed the cowdata-move-constructor branch from ee8bb5e to 5cbf393 Compare December 10, 2024 13:30
@Ivorforce Ivorforce changed the title Add move constructor and move assignment to CowData, String, Char16String, CharString, Vector and Variant. Add move constructor and move assignment to CowData, String, Char16String, CharString, and Vector. Dec 10, 2024
@Ivorforce
Copy link
Contributor Author

I removed the Variant move constructor and assignment because MSVC got confused 🫠
May address that later separately.

@Ivorforce Ivorforce force-pushed the cowdata-move-constructor branch from 5cbf393 to 4cb8e65 Compare December 10, 2024 13:40
@Ivorforce Ivorforce requested a review from kiroxas December 10, 2024 13:40
Copy link
Contributor

@kiroxas kiroxas left a comment

Choose a reason for hiding this comment

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

On a performance standpoint, I think this is good as it will avoid unecessary copies. But because of having to use std::move, it brings some STL headers, which is against the Godot Guidelines, so this needs to be discussed by maintainers, I would think. Move semantic is weird this way as annotation is in the language : the &&, but triggering a move on a named variable isn't std::move being in the library. Maybe you could propose a godot version of move ?

@Ivorforce
Copy link
Contributor Author

But because of having to use std::move, it brings some STL headers, which is against the Godot Guidelines, so this needs to be discussed by maintainers

Notably, std::move is already used in the codebase, but very rarely. The usage guidelines more pertain to using STL, I think, rather than simply including it.

Maybe you could propose a godot version of move ?

I guess that would be possible, but is it really that important to avoid include <utility>?

@kiroxas
Copy link
Contributor

kiroxas commented Dec 10, 2024

But because of having to use std::move, it brings some STL headers, which is against the Godot Guidelines, so this needs to be discussed by maintainers

Notably, std::move is already used in the codebase, but very rarely. The usage guidelines more pertain to using STL, I think, rather than simply including it.

Maybe you could propose a godot version of move ?

I guess that would be possible, but is it really that important to avoid include <utility>?

To be honest I don't know. The only std::move in Godot I found that is not in thirdparty is from one of my PR, interacting with third party code in the engine. I would be for it, but adding it here is really more core, so I guess you'll need maintainers opinions.

@hpvb
Copy link
Member

hpvb commented Dec 10, 2024

We can use things from std::, it is fine. We use std::thread already. The rule only applies to STL containers and such. Using std::move in this context is 100% fine.

@hpvb
Copy link
Member

hpvb commented Dec 10, 2024

All looks really straightforwardly correct, I'll run some tests later but I have no style or technical comments.

Copy link
Contributor

@kiroxas kiroxas left a comment

Choose a reason for hiding this comment

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

If std::move is fine, then this is good. It has some huge speed up potential.

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Yeah, Variant is a fickle and hacky beast, no need to worry about it now

Integrating move constructors/assignment is honestly overdue; I think they're already being utilized in godot-cpp to a limited extent. This implementation makes sense and looks good, nice work!

@clayjohn
Copy link
Member

Just for background. At the contributor meeting this past October we had a long discussion about including certain modern C++ features. We talked about move specifically in detail and the consensus was:

  1. Move can be very useful, especially inside core containers
  2. But it is a fairly niche feature that most contributors aren't familiar with
  3. Therefore, we shouldnt use it wisely across the engine, but should implement it in certain core containers where we can get the most benefit.

I think this PR is in line with our discussion. HP and I also discussed a few other places in core that would benefit from using move.

That being said, we need to be careful about where we use it an how. We shouldn't just be adding move wherever we see a place that it can fit, we should check everything with a profiler and ensure we are actually having an impact and not needlessly making our code difficult to read

@Ivorforce Ivorforce force-pushed the cowdata-move-constructor branch from 4cb8e65 to 57073ba Compare December 11, 2024 14:57
@akien-mga akien-mga requested a review from hpvb December 12, 2024 12:58
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.

Some smoke tests suggest no problems.

@Repiteo Repiteo merged commit 9fba62a into godotengine:master Dec 12, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 12, 2024

Thanks!

@Ivorforce Ivorforce deleted the cowdata-move-constructor branch December 12, 2024 22:35
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.

8 participants