Skip to content

[3.x] Optimize Object::cast_to with ancestral classes when possible #107844

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
merged 1 commit into from
Jun 22, 2025

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Jun 22, 2025

This implementation should be several times faster than regular Object::cast_to, for classes with corresponding ancestral classes (although i have not tested this).

It would be possible to "pre-check" the types that derive from any of the ancestral types (e.g. object && object.has_ancestry(AncestralClass::SPATIAL) && object.is_class_ptr(SpatialSubclass::get_class_ptr_static())). But that would only accelerate the unhappy path, and only some of the time. The happy path wants to immediately use is_class_ptr, so I decided against this micro optimization.

The _is_class function should use constexpr, but it's not possible until the upgrade to C++17.

4.x equivalent will follow for 4.6, when AncestralClass is forward ported.

@Ivorforce Ivorforce added this to the 3.x milestone Jun 22, 2025
@Ivorforce Ivorforce requested review from a team as code owners June 22, 2025 13:13
@Ivorforce Ivorforce requested review from lawnjelly and removed request for a team June 22, 2025 13:13
@Ivorforce Ivorforce force-pushed the 3x-cast-to-ancestral branch 7 times, most recently from 0c09869 to 624c7d0 Compare June 22, 2025 14:13
@Ivorforce Ivorforce force-pushed the 3x-cast-to-ancestral branch from 624c7d0 to aa63595 Compare June 22, 2025 14:21
@lawnjelly lawnjelly mentioned this pull request Jun 22, 2025
@lawnjelly
Copy link
Member

lawnjelly commented Jun 22, 2025

I haven't checked the assembly but I did some timings on this PR with the benchmarks from #107462 .

2000 nodes (repeated 1000 times)
release
ancestry 11294, cast 16772

To give some comparison, the results before the PR were (ratio is important here):
ancestry 175, cast 344

So it indicates we are getting a speedup here, but not as much as calling has_ancestry() directly.

I also tried compiling with c++17, and using:

	if constexpr (T::static_ancestral_class != T::super_type::static_ancestral_class) {
		return has_ancestry(T::static_ancestral_class);
	} else {
		return is_class_ptr(T::get_class_ptr_static());
	}

But that didn't appear to help here.

Perhaps this the cost of the branch in cast_to as opposed to _is_class(). 🤔

I.e. in the benchmark we are checking the pointer is non-NULL for cast_to, versus just checking a bool for has_ancestry(). Something like that. We could be measuring something that doesn't matter in practice. Anyway will check further.

UPDATE:
Yes, I can confirm the difference is only due to the NULL pointer check.
If I change the benchmark from:

if (child->has_ancestry(AncestralClass::SPATIAL)) {
	count++;
}

to

if (child && child->has_ancestry(AncestralClass::SPATIAL)) {
	count++;
}

The results are much more similar:
ancestry 15293, cast 16495

This suggests we'll get a general speedup by using cast_to with this PR, but we should still call has_ancestry() directly (or a wrapper function, so ancestry can remain internal) in certain cases (like checking if something is a Reference when we know it isn't NULL).

@lawnjelly
Copy link
Member

Just to note also that the big change in whitespace in this PR (which causes confusing diff) is due to switching GDCLASS to use GDSOFTCLASS like in 4.x, and also removing is_class_ptr and self_type from GDCLASS.

The main change is:

  • the single static on each class (e.g. static constexpr AncestralClass static_ancestral_class = AncestralClass::REFERENCE;)
  • The change to _is_class() to support ancestry.

@lawnjelly lawnjelly merged commit 3aaabb6 into godotengine:3.x Jun 22, 2025
14 checks passed
@lawnjelly
Copy link
Member

Thanks!

@Ivorforce Ivorforce deleted the 3x-cast-to-ancestral branch June 22, 2025 17:11
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.

2 participants