X Tutup
Skip to content

Avoid unnecessary copy in ClassDB::get_property_list#108504

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
precup:optimize-duplicate
Sep 18, 2025
Merged

Avoid unnecessary copy in ClassDB::get_property_list#108504
Repiteo merged 1 commit intogodotengine:masterfrom
precup:optimize-duplicate

Conversation

@precup
Copy link
Contributor

@precup precup commented Jul 11, 2025

I was poking around the Alloc > Duplicate benchmark and noticed that there's an unnecessary copy happening in ClassDB::get_property_list. validate_property must be run on a copy of the PropertyInfo, but a copy has to be made during push_back anyways, so we can rearrange things a bit and only perform one copy instead of two.

I ran the Alloc > Duplicate benchmark nine times for each and dropped the top two and bottom two of each:

optimize-duplicate
------------------------------
Running benchmark: C++/Alloc/duplicate
Result: {"time":4387.0}
Result: {"time":4422.0}
Result: {"time":4426.0}
Result: {"time":4432.0}
Result: {"time":4443.0}

master
------------------------------
Running benchmark: C++/Alloc/duplicate
Result: {"time":4704.0}
Result: {"time":4710.0}
Result: {"time":4730.0}
Result: {"time":4736.0}
Result: {"time":4764.0}

It's only a ~6.5% speedup, but it's a pretty core function so it seemed worth a PR.

@precup precup requested a review from a team as a code owner July 11, 2025 02:10
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Nice find!

@beicause
Copy link
Contributor

Did you benchmark in release mode? The temporary struct copying here might be optimized and the overhead shouldn't be so noticeable if compiler is smart enough.

@Ivorforce
Copy link
Member

The compiler isn't allowed to make this kind of optimization. It's a good idea to test in release mode but i don't expect this to perform worse there.

@precup
Copy link
Contributor Author

precup commented Jul 11, 2025

Did you benchmark in release mode? The temporary struct copying here might be optimized and the overhead shouldn't be so noticeable if compiler is smart enough.

Good catch, I forgot that I hadn't switched my compiler settings back over. Using optimize=speed_trace:

master
  Result: {"time":1320.0}
  Result: {"time":1230.0}
  Result: {"time":1231.0}
  Result: {"time":1216.0}
  Result: {"time":1229.0}

optimize-duplicate
  Result: {"time":1234.0}
  Result: {"time":1138.0}
  Result: {"time":1146.0}
  Result: {"time":1161.0}
  Result: {"time":1152.0}

I just ran these back to back in one run each, so the slow first one is probably some sort of cache warmup. Seems like a similar level of speed up.

@Repiteo Repiteo merged commit 3fa7c65 into godotengine:master Sep 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 18, 2025

Thanks!

@precup precup deleted the optimize-duplicate branch September 18, 2025 20:38
@akien-mga akien-mga changed the title Avoid unnecessary copy in ClassDB::get_property_list Avoid unnecessary copy in ClassDB::get_property_list Jan 25, 2026
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.

5 participants

X Tutup