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

[3.x] Add move semantics to core containers. #100995

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Jan 1, 2025

Adds to SWAP, Variant, StringName, List, CowData and LocalVector.

Backports:
#100563
#100560
#100483
#100477
#100426
#100367

Notes

  • This backports several of @Ivorforce 's PRs to 3.x.
  • I'm completely new to move semantics so this could do with checking 😁 , especially regarding any differences between 3.x and 4.x.
  • Move semantics seems fairly simple to add and may result in some benefit (same reasoning as for 4.x, discussed in the core meeting). I don't see any obvious reason we shouldn't backport (at least some of) them as they appear fairly simple.
  • This also improves 3.x CowData::insert() which was using set() for each element instead of a one off copy on write via ptrw() (used in 4.x).
  • May make sense for PoolVector too but leaving that for a separate PR as it isn't present in 4.x, and is a bit of a minefield already with some existing thread bugs if I remember right.

Variant

So far no bodges needed for Variant as in 4.x to work around MSVC bug - gdnative etc is completely different in 3.x.

Interestingly, adding the move semantics introduced warnings as _data isn't intialized in 3.x (only the type is set to NIL). I've currently solved this by adding zeroing for the union (which is done in 4.x), although I need to check the benefit of move semantics here outweighs the cost of zeroing the union (versus e.g. silencing the warning) (DONE - see below).

Performance Testing

I've spent a little time doing some rudimentary performance testing.

Startup

Benchmarking startup doesn't seem to show significant differences before / after the PR, for the editor or projects, from simple to TPS demo. Any differences seem within normal variation between runs.

FPS

As expected, in games I didn't notice a difference in FPS, as in the vast majority of cases they are limited by GPU.

Benchmarking GDScript

To try and coax some difference I wrote a CPU intensive script, this did show an improvement.

func my_func(var s):
	return 1

func benchmark():
	
	var before = OS.get_ticks_msec()
	
	var s = 1
	for i in range (100000000):
		s += my_func(s)
 
	var after = OS.get_ticks_msec()

	print(str(after-before) + " ms")

func _ready():
	benchmark()

Before

20107 ms
19860 ms
19858 ms

After

17177 ms
17224 ms
16964 ms
17294 ms

This was with all other apps / windows closed on Linux Mint.

So although I'm not sure these changes will result in noticeable difference in most games, in gdscript intensive tasks at least, there does seem to be improvement, here approx 15%. At a guess this may be the Variant improvements but I haven't profiled.

Build Size

One thing I did notice was a possible increase in build size, I don't know if this has been examined in 4.x.
I'll try and see if this is repeatable, but it may be important in deciding whether to go ahead, particularly for web builds where build size is quite important.

Regular build
370.2 MB, 66.7 MB stripped
This PR
373.1 MB, 67.3 MB stripped

From the artifacts:

Previous commit
android 65.9
ios 20
javascript 10.1
linux-editor-mono 73.2
linux-template-mono 21.3
macos-editor 30.7
macos-template 10.9
windows-editor 36.2
windows-template 12.1

This PR
android 66.3
ios 20
javascript 10.1
linux-editor-mono 73.4
linux-template-mono 21.5
macos-editor 30.7
macos-template 11
windows-editor 36.4
windows-template 12.1

Hmm, looks like javascript is within the margin of 100Kb, so might be okay. Desktop 200Kb is no big deal I guess.

@lawnjelly lawnjelly added this to the 3.7 milestone Jan 1, 2025
@lawnjelly lawnjelly marked this pull request as ready for review January 1, 2025 13:18
@lawnjelly lawnjelly requested a review from a team as a code owner January 1, 2025 13:18
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Looks correct to me! I'm not familiar with how different 3.x is to 4.x, but if the unit tests are passing it's unlikely there are any for containers to require differences in implementation.

I think backporting the change to 3.x is warranted because it's likely we'll achieve more significant speed ups in 4.x with move semantics, some of which may be important enough to backport to 3.x in turn.

Adds to SWAP, Variant, StringName, List, CowData and LocalVector.

Co-Authored-By: Lukas Tenbrink <lukas.tenbrink@gmail.com>
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Didn't re-check the code, but it was fine last time so it should be fine now.
Just wanted to make the checkmark green, in case this is still wanted :)

@akien-mga
Copy link
Member

akien-mga commented Mar 21, 2025

I'm not familiar with how different 3.x is to 4.x, but if the unit tests are passing it's unlikely there are any for containers to require differences in implementation.

Just noting that the unit tests in 3.x are very barebones / practically inexistent so the coverage can't be trusted blindly. The unit test suite in 4.x was a major effort for 4.0 and later which wasn't backported to 3.x.

Not to say that those changes shouldn't be done in 3.x, but the real test will be when users try it.

@lawnjelly lawnjelly merged commit 95a494c into godotengine:3.x Mar 23, 2025
14 checks passed
@lawnjelly
Copy link
Member Author

Thanks!

After having these explained in the core meeting, afaik due to the nature of these changes if they were to fail, they would likely fail straight away (even in the absence of rigorous unit tests in 3.x), and it should be pretty easy to revert if any problems. The more testing we get before the next official build the better imo.

@lawnjelly lawnjelly deleted the move_semantics branch March 23, 2025 10:22
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.

4 participants