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

Move Non Send Resources into thread locals #5135

Closed
wants to merge 6 commits into from

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Jun 29, 2022

Putting this up as a possible fix for non send resource being dropped on the wrong thread. Not sure that this is the direction to take as it significantly complicates the API for non-send resources and has other disadvantages too. See Below. If it is a direction to pursue, this needs additional cleanup, so I've opened it as a draft PR.

Objective

Fix problem with world being send and dropping non send resources on the wrong thread.

Alternative to #3519,

Solution

  • Use the thread_local_object crate to move non send resources into thread local memory. This means that trying to access the non send resource off the thread that spawned it will end up with the resource not being found.
  • thread_local_object drops the objects it keeps when the thread is dropped instead of when it's instance is dropped.
  • remove thread check validation as it's no longer needed.

Advantages

  • Fixes issue with non-send resources being dropped on wrong thread after sending World.
  • This is the idiomatic way in rust to allocate thread local things and drop them on the correct thread. So is less likely to have some weird corner case issue with dropping.
  • Easy to extend to have non send resource on threads other than main thread (Not sure this is useful as all the non-send resources in bevy are non-send, because they need os apis that are only accessible on main thread)

Disadvantages

  • non-send resource api is different from normal resources and increases cognitive burden on users.
  • increases rightward drift
  • There are 2 very similar, but distinct operations. Removing the ThreadLocalResource from the world, world.remove_non_send_resource vs removing the resource from the ThreadLocalResource, ThreadLocalResouce::remove.
  • The stuff referenced by thread_local_object are not dropped when the ThreadLocalResource is dropped. They are only dropped when the thread is dropped or ThreadLocalResource;:remove is called on the corrent thread. I added an explicit drop for the thread that ThreadLocalResource is dropped on, but this doesn't help if the world has been sent. So this is a potential memory leak.
  • probably gives a stronger guarantee than is necessary. We can probably work around the issues by having a send world and a non send world (turtle).
  • adds a level of indirection, which likely hurts performance for accessing the resource

Migration Guide

// TODO

ToDo

  • docs! docs! docs!
  • figure out if there's a better way to ensure that audio is dropped correctly

@james7132
Copy link
Member

We're already using thread_local. Is there a reason to use thread_local_object?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR P-Unsound A bug that results in undefined compiler behavior labels Jun 29, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jun 29, 2022

This appears to be complementary to #3519, correct?

@hymm
Copy link
Contributor Author

hymm commented Jun 29, 2022

We're already using thread_local. Is there a reason to use thread_local_object?

This works very differently despite the similar name. Quoting from the crate:

A thread's values are destroyed when it exits, but the values associated with a ThreadLocal instance are not destroyed when it is dropped. These are in some ways the opposite semantics of those provided by the thread_local crate, where values are cleaned up when a ThreadLocal object is dropped, but not when individual threads exit.

Because of this, this crate is an appropriate choice for use cases where you have long lived ThreadLocal instances which are widely shared among threads that are created and destroyed through the runtime of a program, while the thread_local crate is an appropriate choice for short lived values.

There is also the issue that thread_local expects the types to be Send.

This appears to be complementary to #3519, correct?

Yeah. I'll add it to the description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants