X Tutup
Skip to content

Use AncestralClass to speed up Object::cast_to when possible.#110763

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:object-derives-from
Sep 22, 2025
Merged

Use AncestralClass to speed up Object::cast_to when possible.#110763
Repiteo merged 1 commit intogodotengine:masterfrom
Ivorforce:object-derives-from

Conversation

@Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Sep 21, 2025 •
edited
Loading

This implementation is 2x faster (hot access, probably more on cold access) than regular Object::cast_to, for classes with corresponding ancestral classes.

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.

I also made has_ancestry a private function. Callers should generally use derives_from / Object::cast_to instead, which is more resistant to us changing the ancestral classes in the future.

Benchmark

The benchmarks show a ~2x improvement for cold access. The improvement is likely to be better in reality because we generate more hot situations, and need less cold RAM accesses even in cold situations.

Note that the difference is small in absolute terms, due to cast_to already being quite well optimized.

Object *nav_agent = memnew(NavigationAgent3D);
	Object *node_3D = memnew(Node3D);

	{
		auto t0 = std::chrono::high_resolution_clock::now();
		for (int i = 0; i < 1000000000; i++) {
			// Test happy and optimized
			get_ptr_node3d(node_3D);
		}
		auto t1 = std::chrono::high_resolution_clock::now();
		std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";
	}
	{
		auto t0 = std::chrono::high_resolution_clock::now();
		for (int i = 0; i < 1000000000; i++) {
			// Test happy but unoptimized
			get_ptr_navagent(nav_agent);
		}
		auto t1 = std::chrono::high_resolution_clock::now();
		std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";
	}
	{
		auto t0 = std::chrono::high_resolution_clock::now();
		for (int i = 0; i < 1000000000; i++) {
			// Test unhappy but optimized
			get_ptr_node3d(nav_agent);
		}
		auto t1 = std::chrono::high_resolution_clock::now();
		std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";
	}
	{
		auto t0 = std::chrono::high_resolution_clock::now();
		for (int i = 0; i < 1000000000; i++) {
			// Test unhappy and unoptimized
			get_ptr_navagent(node_3D);
		}
		auto t1 = std::chrono::high_resolution_clock::now();
		std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";
	}

With the helper functions being declared in a separate module (to avoid inlining):

void *get_ptr_node3d(Object *node) { return Object::cast_to<Node3D>(node); }
void *get_ptr_navagent(Object *node) { return Object::cast_to<NavigationAgent3D>(node); }
969ms
2256ms
966ms
2257ms

@Ivorforce Ivorforce added this to the 4.6 milestone Sep 21, 2025
@Ivorforce Ivorforce requested a review from lawnjelly September 21, 2025 23:33
@Ivorforce Ivorforce requested review from a team as code owners September 21, 2025 23:33
@Ivorforce Ivorforce requested review from a team as code owners September 21, 2025 23:33
@Ivorforce Ivorforce force-pushed the object-derives-from branch 2 times, most recently from d3257d6 to ac579a9 Compare September 21, 2025 23:35
@Ivorforce Ivorforce changed the title Use AncestralClass to speed up Object::cast_to when possible. Use AncestralClass to speed up Object::cast_to when possible. Sep 21, 2025
@Ivorforce Ivorforce force-pushed the object-derives-from branch 3 times, most recently from 88a9323 to c5239f6 Compare September 21, 2025 23:56
lawnjelly
lawnjelly approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks fine to me.
Also worth mentioning #107881 which was where we renamed to derives_from in 3.x.

AThousandShips
AThousandShips reviewed Sep 22, 2025
View reviewed changes
@Ivorforce
Copy link
Member Author

Ivorforce commented Sep 22, 2025

For completion's sake, I tested the performance impact of this change. It's about 2x on hot access, but would probably be better on cold access. See main post for details.

@Repiteo Repiteo merged commit 035f5d3 into godotengine:master Sep 22, 2025
57 of 60 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 22, 2025

Thanks!

@Ivorforce Ivorforce deleted the object-derives-from branch September 22, 2025 18:42
@plaught-armor
Copy link

plaught-armor commented Sep 23, 2025

This pull is causing CollisionShape3D errors.

@harrisyu
Copy link
Contributor

harrisyu commented Sep 23, 2025

This pull cause both 3D and 2D collision shape not detect by body.

@lawnjelly
Copy link
Member

lawnjelly commented Sep 23, 2025 •
edited
Loading

Can confirm that removing static_ancestral_class from CollisionObject3D solves the regression (for 3D), probably same for 2D.

Just investigating why it is causing a problem (maybe a conflict for derived classes where it is trying to use ancestry).

I suspect this line isn't working correctly for some inheritance chains, but I haven't figured it out yet:

	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());
	}

Interesting that this problem doesn't seem to occur in 3.x, possibly because we are doing the same thing longhand and checking ancestry directly where it makes sense, or perhaps the inheritance chains are different.

Will leave this to @Ivorforce when he's up.

If he's unavailable today I might be able to have a look at a fix, or we can just revert temporarily to prevent users having broken projects when using master, as the bug is quite annoying.

@lawnjelly
Copy link
Member

lawnjelly commented Sep 23, 2025 •
edited
Loading

Got it, it's actually my bad, rather than a problem with this PR, it's a problem with #107868 (which was merged just before this one, but this one exposed it).

PhysicsBody3D has another constructor that avoids setting the ancestry, this may be a difference between 3.x and 4.x. I'll see if I can fix. (Or it's a fix I made to the 3.x PR but forgot to change on the 4.x PR and they got out of sync).

@lawnjelly lawnjelly mentioned this pull request Sep 23, 2025
Merged
@lawnjelly
Copy link
Member

lawnjelly commented Sep 23, 2025 •
edited
Loading

This pull cause both 3D and 2D collision shape not detect by body.

@harrisyu Although I've confirmed the bug with 3D, I haven't worked out yet how this could be affecting 2D collision shapes, do you have an MRP that shows this? (or even let me know if one of the demos isn't working correctly)

@akien-mga

This comment was marked as resolved.

Sign in to view
@lawnjelly
Copy link
Member

lawnjelly commented Sep 23, 2025

Edit: There doesn't seem to be a PHYSICS_BODY_2D enum value, so I guess it's intended.

Yes, PhysicsBody2D isn't accelerated currently, although CollisionObject2D is (although it looks fine as far as I can see).

The choice of which to accelerate was based on empirical testing, but we may add more in future.

@Ivorforce Ivorforce mentioned this pull request Sep 23, 2025
Merged
@harrisyu
Copy link
Contributor

harrisyu commented Sep 25, 2025

This pull cause both 3D and 2D collision shape not detect by body.

@harrisyu Although I've confirmed the bug with 3D, I haven't worked out yet how this could be affecting 2D collision shapes, do you have an MRP that shows this? (or even let me know if one of the demos isn't working correctly)

Sorry to see that late, I see first pull changed to 2D and 3D, but I only test 3D.
It works fine now with your later fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@lawnjelly lawnjelly lawnjelly approved these changes

Assignees

No one assigned

Labels

enhancement performance topic:core

Projects

None yet

Milestone

4.6

Development

Successfully merging this pull request may close these issues.

7 participants

@Ivorforce @Repiteo @plaught-armor @harrisyu @lawnjelly @akien-mga @AThousandShips
X Tutup