Remove implicit conversions between `LocalVector` and `Vector` #107469

Ivorforce · 2025-06-12T22:01:51Z

Vector copy-conversion to LocalVector, and vice versa, are costly. Conversions are currently implicit, which allows them to happen by accident. This PR changes existing conversions (seems to just be one-way right now) to be explicit instead, preventing accidental performance sinks.

The change exposes current unintended conversions. @godotengine/rendering may want to have a look especially, since there are some conversions that should maybe be addressed.

stuartcarnie · 2025-06-13T01:53:19Z

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp

-	return RD::get_singleton()->framebuffer_format_create(attachments);
+	return RD::get_singleton()->framebuffer_format_create(Vector<RD::AttachmentFormat>(attachments));


This is a great change, as it exposes issues like this, where a thread local variable was used in an effort to improve performance, only to hurt it, as it is immediately copied to the heap.

stuartcarnie

👍🏻 from me, as implicit conversions should be cheap, zero cost IMHO. This exposes situations where an optimisation was intended, but failed due to the implicit conversion.

lawnjelly · 2025-06-13T05:07:05Z

I'll probably do the same in 3.x. I've long thought these implicit conversions were a bad idea especially for heavy types, and I'm sure it will expose some performance bugs.

akien-mga · 2025-06-13T08:09:33Z

I think this might warrant proper review of each occurrence, as it might highlight that some stuff uses LocalVector when it shouldn't. My understanding is that LocalVector was meant to be local and not passed across method boundaries, which typically use Vector. Here it looks like we have code that creates LocalVector only to convert it to Vector whenever it's passed to a method, which seems wasteful.

Ivorforce · 2025-06-13T08:21:35Z

I think this might warrant proper review of each occurrence, as it might highlight that some stuff uses LocalVector when it shouldn't.

Agreed, but unfortunately the fixes might not be fully straight-forward. For example, we had a PR recently that optimized by using LocalVector temporarily and converting to Vector only at the end, because the Vector creation overhead is so large. For example, Vector does not retain its buffer on clear() (yet), which can lead to frequent re-allocations during growth.
I'm guessing some of the performance problems exposed here might be best resolved by making use of Span or VectorView instead.

All in all, personally I'd still move forwards with this one now, and leave it to contributors of the areas to apply an appropriate fix as a follow-up.

akien-mga · 2025-06-13T08:27:00Z

modules/minimp3/audio_stream_mp3.cpp

@@ -239,7 +239,7 @@ void AudioStreamMP3::set_data(const Vector<uint8_t> &p_data) {
 }

 Vector<uint8_t> AudioStreamMP3::get_data() const {
-	return data;
+	return Vector<uint8_t>(data);


There seems to be a conversion from Vector -> LocalVector right above in set_data. Is that direction not handled in this PR?

Because here it seems there's just no point for data to be LocalVector if the input data is Vector and the output should be too. (Unless I guess we're doing stuff with data where LocalVector really gives a huge benefit and warrants this double conversion.)

We do this in the codebase sometimes, where we store LocalVector internally but expose Vector interfaces.
The reason is that Vector still has considerable overhead, even on lookup. If we expect user queries to happen rarely, but internal read/writes to occur often, this type of interface is still preferable, even though we're re-allocating for user queries.

akien-mga · 2025-06-13T08:27:37Z

editor/plugins/skeleton_3d_editor_plugin.cpp

+					surface_tool->set_bones(Vector<int>(bones));
+					surface_tool->set_weights(Vector<float>(weights));


This looks like bones and weightsshould just beVector` in the first place (same for SpringBone3DGizmo).

Edit 2: It looks like the arrays are modified during iteration. In a way, the LocalVector is only used as a blueprint because SurfaceTool receives different buffers during each iteration.
There's probably still optimizations to be done here, but looking at surrounding context I wouldn't feel comfortable simply changing the type above (without testing), due to CoW overheads during the loops, if the arrays are going to be forked each iteration anyway.

akien-mga · 2025-06-13T08:29:03Z

platform/linuxbsd/wayland/wayland_thread.cpp

@@ -101,7 +101,7 @@ Vector<uint8_t> WaylandThread::_read_fd(int fd) {
 		data.resize(bytes_read + chunk_size);
 	}

-	return data;
+	return Vector<uint8_t>(data);


CC @Riteo to confirm that have data as LocalVector to convert on return is worth it here, as opposed to making it Vector from the start (I see it's resized quite a bit so I suppose the current approach is a good option).

I don't really know these classes in depth so I simply followed the advice of "Use LocalVector if you don't need COW".

I see it's resized quite a bit so I suppose the current approach is a good option

Is resizing that expensive with Vector? I'm taking a look at the template and it looks like it has a shortcut for when the refcount is 1, which in this case it would obviously be. If we can save a memcpy I guess we could switch to a Vector outright.

That's just my understanding though.

Yes, anything that writes to Vector (resize especially) is a lot slower with Vector. Whether or not that's important depends on the context of course.

lawnjelly · 2025-06-15T06:07:37Z

BTW something to watch for here which has bit me just now doing conversions from Vector to LocalVector, that the default size is unsigned for LocalVector and signed for Vector (certainly in 3.x, and I think in 4.x) so you might have to check the use cases and convert to unsigned, or make sure they are ok with unsigned counters.

Alternative is to use a LocalVector<T, int32_t> instead of LocalVector<T>.

Ivorforce · 2025-06-15T21:57:25Z

BTW something to watch for here which has bit me just now doing conversions from Vector to LocalVector, that the default size is unsigned for LocalVector and signed for Vector (certainly in 3.x, and I think in 4.x) so you might have to check the use cases and convert to unsigned, or make sure they are ok with unsigned counters.

Alternative is to use a LocalVector<T, int32_t> instead of LocalVector<T>.

In this PR I made no logical changes (just made the conversion explicit), so there should not be any effect on signed-ness of objects' counters.

clayjohn · 2025-06-15T23:45:18Z

For the rendering changes, this looks fine for now. We need to move to using Span more widely so we use LocalVector even more. In particular, the methods here that are converting to Vector should be modified to take a Span, so a LocalVector can be used instead

Ivorforce added this to the 4.x milestone Jun 12, 2025

Ivorforce added the enhancement label Jun 12, 2025

Ivorforce requested a review from a team as a code owner June 12, 2025 22:01

Ivorforce added the topic:core label Jun 12, 2025

Ivorforce requested review from a team as code owners June 12, 2025 22:01

Ivorforce added the topic:codestyle label Jun 12, 2025

Ivorforce requested review from a team as code owners June 12, 2025 22:01

Ivorforce removed request for a team June 12, 2025 22:02

Ivorforce changed the title ~~Change Vector <-> LocalVector conversions to be Span based, and make them explicit~~ Remove implicit conversions between LocalVector and Vector Jun 12, 2025

Ivorforce force-pushed the vector-localvector-explicit-span-conversions branch from a693e80 to 71b62a8 Compare June 12, 2025 22:38

Ivorforce requested a review from a team as a code owner June 12, 2025 22:38

Ivorforce removed the request for review from a team June 12, 2025 22:54

Calinou added the performance label Jun 12, 2025

Make conversions from LocalVector to Vector explicit.

b63645c

Ivorforce force-pushed the vector-localvector-explicit-span-conversions branch from 71b62a8 to b63645c Compare June 12, 2025 23:42

stuartcarnie reviewed Jun 13, 2025

View reviewed changes

stuartcarnie self-requested a review June 13, 2025 01:56

stuartcarnie approved these changes Jun 13, 2025

View reviewed changes

akien-mga reviewed Jun 13, 2025

View reviewed changes

Repiteo modified the milestones: 4.x, 4.5 Jun 13, 2025

akien-mga modified the milestones: 4.5, 4.x Jun 13, 2025

lawnjelly mentioned this pull request Jun 14, 2025

[3.x] Remove implicit conversion from LocalVector to Vector #107543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Remove implicit conversions between `LocalVector` and `Vector` #107469

Remove implicit conversions between `LocalVector` and `Vector` #107469

Ivorforce commented Jun 12, 2025 •

edited

Loading

Uh oh!

stuartcarnie Jun 13, 2025

Uh oh!

stuartcarnie left a comment

Uh oh!

lawnjelly commented Jun 13, 2025

Uh oh!

akien-mga commented Jun 13, 2025 •

edited

Loading

Uh oh!

Ivorforce commented Jun 13, 2025 •

edited

Loading

Uh oh!

akien-mga Jun 13, 2025

Uh oh!

Ivorforce Jun 13, 2025

Uh oh!

akien-mga Jun 13, 2025

Uh oh!

Ivorforce Jun 13, 2025 •

edited

Loading

Uh oh!

akien-mga Jun 13, 2025

Uh oh!

Riteo Jun 13, 2025

Uh oh!

Ivorforce Jun 16, 2025

Uh oh!

lawnjelly commented Jun 15, 2025

Uh oh!

Ivorforce commented Jun 15, 2025 •

edited

Loading

Uh oh!

clayjohn commented Jun 15, 2025

Uh oh!

Uh oh!

		return RD::get_singleton()->framebuffer_format_create(attachments);
		return RD::get_singleton()->framebuffer_format_create(Vector<RD::AttachmentFormat>(attachments));

		surface_tool->set_bones(Vector<int>(bones));
		surface_tool->set_weights(Vector<float>(weights));

Uh oh!

Remove implicit conversions between LocalVector and Vector #107469

Are you sure you want to change the base?

Remove implicit conversions between LocalVector and Vector #107469

Conversation

Ivorforce commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartcarnie Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

stuartcarnie left a comment

Choose a reason for hiding this comment

Uh oh!

lawnjelly commented Jun 13, 2025

Uh oh!

akien-mga commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivorforce commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akien-mga Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Ivorforce Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

akien-mga Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Ivorforce Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akien-mga Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Riteo Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Ivorforce Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

lawnjelly commented Jun 15, 2025

Uh oh!

Ivorforce commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayjohn commented Jun 15, 2025

Uh oh!

Uh oh!

Remove implicit conversions between `LocalVector` and `Vector` #107469

Remove implicit conversions between `LocalVector` and `Vector` #107469

Ivorforce commented Jun 12, 2025 •

edited

Loading

akien-mga commented Jun 13, 2025 •

edited

Loading

Ivorforce commented Jun 13, 2025 •

edited

Loading

Ivorforce Jun 13, 2025 •

edited

Loading

Ivorforce commented Jun 15, 2025 •

edited

Loading