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 LocalVector move semantics (constructor and operator=). #100560

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 18, 2024

We've recently begun to introduce move semantics to core, for its performance benefits. Examples of functions that can benefit a lot from move semantics include SWAP (#100367), reduce_at and insert (#100477), and simple data transfer. For more information on motivation, see #100426.

For LocalVector, this is especially important because a copy-constructor and copy-assignment is forced to copy the entire array, and all elements in it.

Examples

This time I've gathered relevant beneficiaries through a [[deprecated]] trick. Here are some examples:

LocalVector<uint32_t> simplified_path_indices;
simplified_path_indices.reserve(p_path.size());
simplified_path_indices.push_back(0);
simplify_path_segment(0, p_path.size() - 1, p_path, squared_epsilon, simplified_path_indices);
simplified_path_indices.push_back(p_path.size() - 1);
return simplified_path_indices;

(the memory no longer has to be re-allocated to return the vector)

The same was happening in ShaderPreprocessor::advance:

SWAP calls also required double reallocations in e.g. OAHashMap on insert:

SWAP(hash, hashes[pos]);
SWAP(key, keys[pos]);
SWAP(value, values[pos]);

e.g .from ShaderGLES3::Version::Specialization items:

struct Specialization {
GLuint id;
GLuint vert_id;
GLuint frag_id;
LocalVector<GLint> uniform_location;
LocalVector<GLint> texture_uniform_locations;

There's some more examples but I think that should be enough evidence that the addition is warranted :)

hpvb

This comment was marked as outdated.

hpvb

This comment was marked as outdated.

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, and part of the core types we want to have this property.

@Ivorforce Ivorforce force-pushed the localvector-move-semantics branch from 07b6863 to 3564e7c Compare December 19, 2024 14:05
@Repiteo Repiteo merged commit 0b01f3c into godotengine:master Dec 20, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

@Ivorforce Ivorforce deleted the localvector-move-semantics branch December 20, 2024 08:42
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.

6 participants