X Tutup
Skip to content

GDScript: Elide unnecessary copies in CONSTRUCT_TYPED_* opcodes#110717

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
Shadows-of-Fire:elide-copy-typed-collection
Sep 23, 2025
Merged

GDScript: Elide unnecessary copies in CONSTRUCT_TYPED_* opcodes#110717
Repiteo merged 1 commit intogodotengine:masterfrom
Shadows-of-Fire:elide-copy-typed-collection

Conversation

@Shadows-of-Fire
Copy link
Contributor

@Shadows-of-Fire Shadows-of-Fire commented Sep 20, 2025

This PR fixes an issue that was noticed by @AdriaandeJongh while looking at the benchmark results used in #110709

In those benchmark results, the time it took to use initializer-list syntax to create a typed dictionary with 64 entries was 3x as long as the time it took to create an untyped dictionary.

This is because instead of using set_typed, both OPCODE_CONSTRUCT_TYPED_ARRAY and OPCODE_CONSTRUCT_TYPED_DICTIONARY first construct an untyped dictionary, fill it, and use the copy constructor that accepts type parameters to create a typed copy of the dictionary.

By migrating these to use set_typed instead, and re-running the benchmark from #110709, the time for typed and untyped dictionaries is effectively identical.

@Shadows-of-Fire Shadows-of-Fire requested a review from a team as a code owner September 20, 2025 04:52
@Shadows-of-Fire Shadows-of-Fire changed the title Elide unnecessary copies in CONSTRUCT_TYPED_* opcodes GDScript: Elide unnecessary copies in CONSTRUCT_TYPED_* opcodes Sep 20, 2025
@dalexeev dalexeev added bug topic:gdscript performance cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release labels Sep 20, 2025
@dalexeev dalexeev added this to the 4.6 milestone Sep 20, 2025
dalexeev
dalexeev approved these changes Sep 20, 2025
View reviewed changes
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

I haven't tested this, but it looks right to me. Good catch! This might change some error messages and possibly cause tests to fail, but that's okay.

Shadows-of-Fire
Shadows-of-Fire commented Sep 20, 2025
View reviewed changes
@Shadows-of-Fire
Copy link
Contributor Author

Shadows-of-Fire commented Sep 20, 2025

The error messages in the tests that appear to be impacted from this change are surprisingly unhelpful. It seems like utils.notest.gd::check(condition) is unable to actually print a backtrace, and only produces Check failed. Backtrace (most recent call first):.

Going to have to fiddle around to see why, I guess.

@Shadows-of-Fire
Copy link
Contributor Author

Shadows-of-Fire commented Sep 20, 2025

Okay, had to find/replace uses of Utils.check in the failing tests with assert (seemed to be the fastest way to get line data without fighting check).

One of the failing tests is this hunk from typed_dictionary_usage.gd:

	var typed_int := 556
	var converted_floats: Dictionary[float, float] = { typed_int: typed_int }
	converted_floats[498.0] = 894
	assert(str(converted_floats) == '{ 556.0: 556.0, 498.0: 894.0 }')

This fails because of this deviation:

real:      { 556.0: 556, 498.0: 894.0 }
expected:  { 556.0: 556.0, 498.0: 894.0 }

Note the missing .0 for the value 556. Does this imply that when using initializer-list syntax with set_typed that the keys are converted but not the values?

Or is this some kind of floating-point printing error

@Shadows-of-Fire
Copy link
Contributor Author

Shadows-of-Fire commented Sep 20, 2025

Well, I guess that answers my question, lol

// WARNING: This operator does not validate the value type. For scripting/extensions this is
// done in `variant_setget.cpp`. Consider using `set()` if the data might be invalid.
Variant &Dictionary::operator[](const Variant &p_key) {

Guessing the case is the same for Array then.

@Shadows-of-Fire
Copy link
Contributor Author

Shadows-of-Fire commented Sep 20, 2025 •
edited
Loading

Well, at least for the dictionary case, using Dictionary::set seems to handle it. The perf is a little bit worse than operator[], but still substantially better than the copy case.

From the benchmark in #110709 (but without the reserve() changes) -

Time with `set_typed` and `set` (this change after last commit)
time untyped: 17660ms
time typed: 19050ms

Time with copy and `operator[]` (before this change)
time untyped: 17533ms
time typed: 45386ms

@Shadows-of-Fire Shadows-of-Fire force-pushed the elide-copy-typed-collection branch from 0badca0 to b18beb2 Compare September 20, 2025 07:00
@Shadows-of-Fire Shadows-of-Fire requested a review from a team as a code owner September 20, 2025 07:00
Ivorforce
Ivorforce reviewed Sep 20, 2025
View reviewed changes
modules/gdscript/gdscript_vm.cpp
const StringName native_type = _global_names_ptr[native_type_idx];

Array array;
array.set_typed(builtin_type, native_type, *script_type);
Copy link
Member

@Ivorforce Ivorforce Sep 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note that typed array resize needs to construct all elements as empty elements of the needed type. And then .set needs to replace them with the actual values.

It might still be faster than repeated push_back calls, but I'm guessing not by as much as might be expected. This should probably be tested.
Once Array gets reserve (scheduled for early 4.6 dev), I'm pretty sure the reserve + push_back approach should be faster than resize + set.

Copy link
Contributor Author

@Shadows-of-Fire Shadows-of-Fire Sep 21, 2025

Choose a reason for hiding this comment

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

I ran a similar test to the dictionary one with an array of length 40; the time difference between the approaches for Array is negligible. In both cases the typed array is about 50% slower than the untyped array.

Prior to this change:
time untyped: 2396ms
time typed: 3691ms

After this change:
time untyped: 2400ms
time typed: 3542ms

So it's ever so slightly faster; enough to mean we don't need to explicitly hold this change for reserve, but it would be better if reserve was present.

Copy link
Member

@Ivorforce Ivorforce Sep 21, 2025

Choose a reason for hiding this comment

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

Ok, thanks for testing!
Personally, since both approaches are the same speed, I'd take this as an indication to use push_back instead, since it's simpler than resize and set. Plus, we'll be able to insert reserve when it's here. But either would be fine for me right now, since we normally don't plan for future code changes.

@Shadows-of-Fire
Copy link
Contributor Author

Shadows-of-Fire commented Sep 21, 2025

Not sure what happened with the Windows / Template CI job there. Is that an infra issue? Looks like it just did nothing for 2 hours after running ./bin/godot.windows.template_release.x86_64.console.exe --help

@Repiteo Repiteo merged commit aa2c4fe into godotengine:master Sep 23, 2025
38 of 40 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 23, 2025

Thanks! Congratulations on your first merged contribution! 🎉

@DeeJayLSP DeeJayLSP mentioned this pull request Sep 23, 2025
Merged
@matheusmdx matheusmdx mentioned this pull request Jan 8, 2026
Closed
@akien-mga akien-mga removed the cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release label Jan 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ivorforce Ivorforce Ivorforce left review comments

@dalexeev dalexeev dalexeev approved these changes

Assignees

No one assigned

Labels

bug performance topic:gdscript

Projects

None yet

Milestone

4.6

Development

Successfully merging this pull request may close these issues.

5 participants

@Shadows-of-Fire @Repiteo @Ivorforce @dalexeev @akien-mga
X Tutup