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.

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

Make RID_Owner lock-free for fetching. #97465

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

DarioSamo
Copy link
Contributor

Taking over #86333 from @reduz to fix some of the newest conflicts.

@akien-mga akien-mga added this to the 4.4 milestone Sep 25, 2024
@akien-mga akien-mga changed the title Make RID_Owner lock-free for fetching. Make RID_Owner lock-free for fetching. Sep 25, 2024
@akien-mga akien-mga added enhancement and removed bug labels Sep 25, 2024
core/templates/rid_owner.h Outdated Show resolved Hide resolved
core/templates/rid_owner.h Outdated Show resolved Hide resolved
core/templates/rid_owner.h Outdated Show resolved Hide resolved
This PR makes RID_Owner lock free for fetching values, this should give a very
significant peformance boost where used.

Some considerations:

* A maximum number of elements to alocate must be given (by default 256k).
* Access to the RID structure is still safe given they are independent from addition/removals.
* RID access was never really thread-safe in the sense that the contents of the data are not protected anyway. Each server needs to implement locking as it sees fit.
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

As far as I can tell, some extra memory ordering enforcement is needed here. That said, I haven't been able to make this misbehave on ARM, the weakest ordering arch we're targeting, in the experiments I'm doing in #97654. I may make a future PR adding those even if it's only for theoretical correctness or future-proofing.

For now, I can't but greenlight this.

@akien-mga akien-mga merged commit 8e6ade8 into godotengine:master Oct 4, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

5 participants