Skip to content

Compile out editor-only logic within _validate_property in export template #105907

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

Merged

Conversation

beicause
Copy link
Contributor

@beicause beicause commented Apr 29, 2025

Addresses #103357.
Wraps editor-only logic in validate_property with Engine::get_singleton()->is_editor_hint() or

if (!Engine::get_singleton()->is_editor_hint()) {
    return;
}

to make it only run in the editor (mainly PROPERTY_USAGE_NO_EDITOR PROPERTY_USAGE_READ_ONLY and property hint ,except PROPERTY_USAGE_NONE, PROPERTY_USAGE_STORAGE).
This speeds up Object.get_property_list Resource.duplicate and Node.duplicate for certain classes that do many string comparisons in validate property, and should also speed up scene loading.

Benchmark:

func _ready() -> void:
	var m := StandardMaterial3D.new()
	var e := Environment.new()

	var t := Time.get_ticks_msec()
	for i in range(1000):
		m.duplicate()
	var t1 := Time.get_ticks_msec()

	print(t1 - t)

	t = Time.get_ticks_msec()
	for i in range(1000):
		e.duplicate()
	t1 = Time.get_ticks_msec()

	print(t1 - t)

	t = Time.get_ticks_msec()

	for i in range(1000):
		m.get_property_list()
	t1 = Time.get_ticks_msec()

	print(t1 - t)

	t = Time.get_ticks_msec()
	for i in range(1000):
		e.get_property_list()
	t1 = Time.get_ticks_msec()

	print(t1 - t)

Edit:
In editor: 200 105 452 360
In game:
before: 124 107 241 217
after: 57 34 186 132

@beicause beicause requested review from a team as code owners April 29, 2025 14:04
@dsnopek
Copy link
Contributor

dsnopek commented Apr 29, 2025

Thanks, this makes sense to me in principle.

I'm assuming the benchmarks in the description are with this PR? Can you also give benchmarks without this PR to compare against?

@Calinou
Copy link
Member

Calinou commented Apr 29, 2025

This could also help with binary size significantly.

@beicause
Copy link
Contributor Author

Recompiled and tested. Indeed, it is still faster in the game than in the editor without this PR, but this PR still has significant improvement.

without this PR:
In editor: 207 112 498 386
In game: 122 106 247 201

with this PR:
In editor: 211 112 502 382
In game: 66 37 204 155

@beicause beicause force-pushed the compile-out-validate-property branch from e3ca5ea to ebf5dd6 Compare April 29, 2025 17:32
@arkology
Copy link
Contributor

Interesting, is the TOOLS_ENABLED check actually necessary? I thought compilers were smart enough to optimize out if (false) {} (as is_editor_hint() returns false for non-editor builds - but maybe compilers don't expand Engine::get_singleton()->is_editor_hint() to false during compilation?).

@beicause beicause force-pushed the compile-out-validate-property branch from ebf5dd6 to 0fc9fb5 Compare May 3, 2025 04:16
@beicause
Copy link
Contributor Author

beicause commented May 3, 2025

Interesting, is the TOOLS_ENABLED check actually necessary? I thought compilers were smart enough to optimize out if (false) {} (as is_editor_hint() returns false for non-editor builds - but maybe compilers don't expand Engine::get_singleton()->is_editor_hint() to false during compilation?).

After this PR, the size of linux release template is reduced by 31KB. Removing the TOOLS_ENABLED doesn't affect the size, so I believe they are still compiled out without TOOLS_ENABLED.

Not sure if we should remove the TOOLS_ENABLED. I found most of the other Engine::get_singleton()->is_editor_hint() in the codebase are not surrounded by TOOLS_ENABLED, but there are also some exceptions.

@beicause beicause force-pushed the compile-out-validate-property branch from 0fc9fb5 to b5c48f5 Compare May 19, 2025 13:50
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I checked all changes and they are correct.

Though I still think that methods completely wrapped in is_editor_hint() should just use early return. Partially disabled methods are fine, early return would be error-prone in their case.

@beicause beicause force-pushed the compile-out-validate-property branch 2 times, most recently from d95d632 to e28be82 Compare May 20, 2025 06:07
@beicause beicause force-pushed the compile-out-validate-property branch 2 times, most recently from 2443622 to 0f09862 Compare May 20, 2025 06:23
@beicause beicause force-pushed the compile-out-validate-property branch from 0f09862 to 2c4e790 Compare May 20, 2025 06:33
@beicause
Copy link
Contributor Author

I have changed to use return for completely wrapped methods, but I will check it again later.

@beicause
Copy link
Contributor Author

beicause commented May 20, 2025

I'm surprised that we use the  ^=  operator in  _validate_property, which means the results will be different when it runs repeatedly. Is this a bug?

@KoBeWi
Copy link
Member

KoBeWi commented May 20, 2025

_validate_property() runs on a copy of PropertyInfo, so each validation is independent.

@TokageItLab
Copy link
Member

Now this PR should be rebased.

@beicause beicause force-pushed the compile-out-validate-property branch from 73eeea1 to 352b135 Compare June 4, 2025 05:23
@beicause beicause force-pushed the compile-out-validate-property branch from 352b135 to 2fbd799 Compare June 4, 2025 11:24
@akien-mga akien-mga requested a review from TokageItLab June 5, 2025 09:21
@akien-mga akien-mga modified the milestones: 4.x, 4.5 Jun 5, 2025
@beicause beicause force-pushed the compile-out-validate-property branch 8 times, most recently from 57f81be to 6de8252 Compare June 12, 2025 02:17
@beicause beicause force-pushed the compile-out-validate-property branch from 6de8252 to 8ba4656 Compare June 12, 2025 04:55
@beicause
Copy link
Contributor Author

Rebased and checked all _validate_property again.

@akien-mga akien-mga merged commit 75afec8 into godotengine:master Jun 12, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@beicause beicause deleted the compile-out-validate-property branch June 12, 2025 10:23
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.

8 participants