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.

Sign up for GitHub

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

Jump to bottom

Use increment instead of conditional_increment for SafeRefCount. #102977

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

Use increment instead of conditional_increment for SafeRefCount. #102977

wants to merge 1 commit into from

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Feb 17, 2025 •
edited
Loading

Closes godotengine/godot-proposals#11795.

This simplifies SafeRefCount::ref calls to always succeed, eliminating logic.

In particular, this PR will speed up RefCounted::init_ref calls (now reference), e.g. improving GDScript function call overhead by ~4%.

It should also slightly improve speed and reduce code of reference calls in general.

Caveats

This commit introduces an edge case where the 2^32th reference will result in rollover, accidentally deallocating the object in question early.
It is unlikely this edge case is ever triggered for a real scenario, because 2^32 references would be 32GB of packed references (â 8 bytes), which is obviously nuts.

@Ivorforce Ivorforce requested review from a team as code owners February 17, 2025 23:18
@Repiteo Repiteo added this to the 4.x milestone Feb 18, 2025
@Ivorforce
Use increment instead of conditional_increment for SafeRefCount.
a4c0343 
This simplifies `ref` calls to always succeed, eliminating logic. This commit introduces an edge case where the 2^32th reference will result in rollover, accidentally deallocating the object in question.
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Feb 21, 2025 •
edited
Loading

I just thought of an edge case with WeakRef. I think its implementation is already flawed in its current form:

Currently, WeakRef asks the ObjectDB to resolve the ID to a reference. Then, it assumes the Object is still in memory, and attempts to reference it:

  • If RefCounted is still alive, it references and returns it.
  • If RefCounted is destructing, but still alive, refcount is already 0 and it refuses to reference (correct).
  • If RefCounted is destructing, but its memory overwritten between the ObjectDB query and reference call, we have UB and probably a crash.

The last case is rare, but I'm pretty sure it's an active bug, and may actually lead to random crashes right now, if WeakRef is used extensively.

My implementation would make it worse, because it changes the middle case to succeed incorrectly, letting the owner upgrade to a Ref to a dead object.

Before merging this PR, this bug should be fixed. I'm not sure what the best approach would be, but I'm still sure not allowing reference from 0 is not the ideal approach. Probably, it would be the responsibility of Ref to delete the ObjectDB entry, and waiting for a positive approval, before starting to destroy the object. In turn, ObjectDB would be responsible for incrementing refcount under lock.

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

@RandomShaper RandomShaper Awaiting requested review from RandomShaper

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

Use increment instead of conditional_increment for SafeRefCount
2 participants
@Ivorforce @Repiteo