Skip to content

Provide quick access to Object ancestry #107868

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Jun 22, 2025

Forward port of #107462
See that PR for full details.

TLDR this internal functionality should speed up things like object casting by approx 2x in most cases.

Introduction

Although we have increased Object::cast_to performance significantly with #103708 (4.x) and #104825 (3.x) we discussed at the time that for some high performance bottleneck scenarios we may want an even faster way of determining whether an Object is one of the key main types.

At the time I trialed using a bitfield to store this info and it worked well, and is likely to be one of the fastest methods, and discussed this with @Ivorforce .

While it involves storing (and retrieving) data from the Object / Node (thus cache effects), it avoids overheads with a virtual function approach, and the virtual function requires reading the vtable from the object, so there is a read in all cases.

Benchmarks

2000 node children

release
ancestry 175, virtual 297, cast_to 344
debug
ancestry 2719, virtual 2804, cast_to 3386

Usage

Although ancestry can be used directly, the plan is (as with 3.x):

  • Use ancestry mostly internally via Object::cast_to (no change to existing code).
  • We also may rename Object::_is_class to Object::derives_from for some other use cases (already done in 3.x in [3.x] Optimize hotspots with Object::derives_from #107881), so that ancestry can then be hidden and only used internally.

Notes

  • Only having a single usage so far to keep the PR simple, @Ivorforce can do a follow up to convert cast_to to use Ancestry for the specific cases covered, so it will invisibly be used everywhere (see [3.x] Optimize Object::cast_to with ancestral classes when possible #107844 for 3.x version).
  • The single case I've used here is replacing the type_is_reference flag on Object, as that is no longer necessary now we have AncestralClass::REF_COUNTED.
  • I didn't attempt to get rid of the peculiar arrangement for the Object constructor, which seems to be in order to accommodate RefCounted. I worked out that this seems to because add_instance() requires being able to check whether the object is a reference during the call. In the long term we should probably see if calling that can be done later in the RefCounted constructor, so the Object constructor can be kept simple.

@lawnjelly lawnjelly requested a review from Ivorforce June 23, 2025 06:22
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 good to me!

We've notably discussed this feature in a core meeting as well, and there were no reservations against merging this feature. It really is basically free, at least until we reach a bit field of a total 32 bits.

Comment on lines +2211 to +2212
// ObjectDB::add_instance relies on AncestralClass::REF_COUNTED
// being already set in the case of references.
Copy link
Member

Choose a reason for hiding this comment

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

This has always irked me. I'm very unconvinced an Object should add itself to the DB before it's even done self-constructing. But it's not for this PR to fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, I thought the same thing. There's also the possibility that other things haven't finished setting up before this call.

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