Skip to content

Make RID_Owner lock-free for fetching.#97465

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
DarioSamo:lock-free-rid
Oct 4, 2024
Merged

Make RID_Owner lock-free for fetching.#97465
akien-mga merged 1 commit into
godotengine:masterfrom
DarioSamo:lock-free-rid

Conversation

@DarioSamo

Copy link
Copy Markdown
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
Comment thread core/templates/rid_owner.h Outdated
Comment thread core/templates/rid_owner.h Outdated
Comment thread core/templates/rid_owner.h Outdated
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.

@RandomShaper RandomShaper left a comment

Copy link
Copy Markdown
Member

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
@akien-mga

Copy link
Copy Markdown
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