Add move semantics (constructor, assignment) to StringName.

[Ivorforce]

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.

[hpvb]

LGTM

[clayjohn]

Now that we are getting into territory where the performance benefit is less obvious, i think we need to start profiling to show that this actually makes a measurable difference.

[Ivorforce]

Now that we are getting into territory where the performance benefit is less obvious, i think we need to start profiling to show that this actually makes a measurable difference.

Generally I agree; not every type needs move semantics.
If you insist, I can run a benchmark on remove_at and insert as I did for #100477. But I'm fairly certain it's not needed; we should expect at least a ~2x improvement here too.

[clayjohn]

Now that we are getting into territory where the performance benefit is less obvious, i think we need to start profiling to show that this actually makes a measurable difference.

Generally I agree; not every type needs move semantics. If you insist, I can run a benchmark on remove_at and insert as I did for #100477. But I'm fairly certain it's not needed; we should expect at least a ~2x improvement here too.

Profiling is not the same as benchmarking. Profiling means finding a place in Godot that triggers this case often and checking to see that you have saved time by making this change. Remember, the end goal for every performance PR is to make Godot games run faster. We need to actually show that this change will help games run faster.

[Ivorforce]

Now that we are getting into territory where the performance benefit is less obvious, i think we need to start profiling to show that this actually makes a measurable difference.

Generally I agree; not every type needs move semantics. If you insist, I can run a benchmark on remove_at and insert as I did for #100477. But I'm fairly certain it's not needed; we should expect at least a ~2x improvement here too.

Profiling is not the same as benchmarking. Profiling means finding a place in Godot that triggers this case often and checking to see that you have saved time by making this change. Remember, the end goal for every performance PR is to make Godot games run faster. We need to actually show that this change will help games run faster.

Well, besides the 8500 uses of the StringName word across cpp and h files (vs about 22k for Variant, btw.), isn't potential for future acceleration reason enough to add move semantics? There are entire categories of acceleration that simply cannot be performed without these functions, such as move-semantics insert and remove_at mentioned above. Like, sure, maybe we don't need a move constructor for SpriteFramesEditor, but I think for the types as critical to the engine as StringName, we should do our best to make them as fast and idiomatic as possible, for benefits that span the entire codebase, as well as unlocking further improvements that simply cannot be done without them.

If you still think it's not warranted without a specific use case, I'm sure I can find something in the codebase that is actually, practically improved by this... But is it really worth it for 14 lines of pretty trivial code?

[kiroxas]

This make sense for core data structures.

[kiroxas]

if (_data != p_name._data) {
	unref();
	_data = p_name._data;
	p_name._data = nullptr;
}
return *this;

[kiroxas]

Maybe assign nullptr to p_name in both cases ? Or else the caller is still valid after the call, which is fine by move standard, but could be error prone.

[Ivorforce]

nullptr cannot be assigned to the caller because the if (_data != p_name._data) { test covers this == &p_name - so we could accidentally be setting our own pointer to nullptr accidentally. Besides, if we set its data to nullptr without taking ownership of it, we would need to unref it. move semantics aren't guaranteed to destruct the param anyway, but they are guaranteed to keep it in a valid state.
Regarding your code change suggestion, like last time I still prefer early exit to indented blocks :)

[kiroxas]

That's style anyway, so any will do, but you already have the indented block :). This just remove one line.

[Ivorforce]

Yeah - just to clear it up i prefer indented blocks to be the trivial path and unindented blocks to be the important path. This principle allows you to always know which is the important path, and it helps prevent double indentations or reformats if another early exit case pops up.

[AThousandShips]

That's the general principle we go by, early exit is preferred

[clayjohn]

Well, besides the 8500 uses of the StringName word across cpp and h files (vs about 22k for Variant, btw.), isn't potential for future acceleration reason enough to add move semantics? There are entire categories of acceleration that simply cannot be performed without these functions, such as move-semantics insert and remove_at mentioned above. Like, sure, maybe we don't need a move constructor for SpriteFramesEditor, but I think for the types as critical to the engine as StringName, we should do our best to make them as fast and idiomatic as possible, for benefits that span the entire codebase, as well as unlocking further improvements that simply cannot be done without them.

The road to bloat is paved with good intentions. potential for improvement is not the same thing as improvement. Idiomatic code isn't a benefit in itself. If a code change improves performance, then its not a burden to show that instead of just saying it.

Remember, the decision of the maintainers was that we shouldn't use move semantics widely since it makes the codebase harder to understand and more difficult to attract new contributors. Optimizing for maintainability is our number one priority. So if we are going to make something harder to understand and harder to maintain, we have to justify it.

[AThousandShips]

LGTM

[hpvb]

The road to bloat is paved with good intentions. potential for improvement is not the same thing as improvement. Idiomatic code isn't a benefit in itself. If a code change improves performance, then its not a burden to show that instead of just saying it.

Remember, the decision of the maintainers was that we shouldn't use move semantics widely since it makes the codebase harder to understand and more difficult to attract new contributors. Optimizing for maintainability is our number one priority. So if we are going to make something harder to understand and harder to maintain, we have to justify it.

I agree with this 100%, however in this case given how very pervasive StringName is, and how often it is part of other structures, not putting move constructors and assignment operators in StringName will, in my mind, certainly prevent the compiler from being allowed to use moves at times where they would be very beneficial. I think we need them for String, StringName, our containers, core math types (using memcpy instead of 3 or 4 assignments), the use of unions makes these a weird one for the compiler probably, and variant.

I think once those are in place, adding more would indeed just be useless for performance unless a very clear (profile based!) case can be made for them.

[hpvb]

@clayjohn does my comment address your concern? If so I'd like to see this merged so we can proceed with this sub project :)

[clayjohn]

@clayjohn does my comment address your concern? If so I'd like to see this merged so we can proceed with this sub project :)

Yes :)

Additionally, we discussed this (and the other move semantics PRs) in a maintainer meeting today and agreed that we are happy to add move semantics to most core types as long as the implementation is relatively straightforward and doesn't require changes in how the type is used outside of core. This is a bit broader than our agreement from October.

[Repiteo]

Thanks!