bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Published 20 hours ago • verified publisher icon bevyengine

Remove `Sync` bound from `Local`

Open PROMETHIA-27 opened this issue 1 year ago • 2 comments

Objective

Currently, Local has a Sync bound. Theoretically this is unnecessary as a local can only ever be accessed from its own system, ensuring exclusive access on one thread. This PR removes this restriction.

Solution

  • By removing the Resource bound from Local and adding the new SyncCell threading primative, Local can have the Sync bound removed.

Changelog

Added

  • Added SyncCell to bevy_utils

Changed

  • Removed Resource bound from Local
  • Local is now wrapped in a SyncCell

Migration Guide

  • Any code relying on Local<T> having T: Resource will have to be changed, but this is unlikely.

PROMETHIA-27 avatar Jul 29 '22 14:07 PROMETHIA-27

bors try

mockersf avatar Jul 29 '22 15:07 mockersf

try

Build succeeded:

bors[bot] avatar Jul 29 '22 16:07 bors[bot]

I like the relaxation on the bounds for Locals, but I'm unsure about taking on SyncCell when Exclusive seems to be close-ish to being stabilized. Are there any clear use cases for this? If there are it'd probably be worth merging now.

hymm avatar Sep 01 '22 21:09 hymm

The idea is to just swap SyncCell for Exclusive once it stabilizes. As this is probably going to be the only use of SyncCell until that point, it shouldn't take more than a minute to put together a PR for.

PROMETHIA-27 avatar Sep 01 '22 21:09 PROMETHIA-27

users could also depend on it. see the issue on bevy_fundsp that is in this feed.

hymm avatar Sep 01 '22 21:09 hymm

In that case, we can typedef SyncCell to Exclusive and deprecate it, and remove it in the next major version. Or just deprecate it. Or just remove it- bevy users are still very much supposed to expect breaking changes, as long as we add to the migration guide for that version that SyncCell is replaced by Exclusive it should be fine. Either way I don't see it as worth waiting for something to stabilize "soon" unless we have a concrete date.

PROMETHIA-27 avatar Sep 01 '22 21:09 PROMETHIA-27

see the issue on bevy_fundsp that is in this feed.

btw, i don't need it anymore as #5819 is now merged.

also, if in the case that users would need it, i would prefer just removing it outright as bevy's api isn't really stabilized yet. I think bevy's development is fast enough to update SyncCell usage to Exclusive, and we don't have to wait for stabilization.

harudagondi avatar Sep 01 '22 22:09 harudagondi

bors r+

alice-i-cecile avatar Sep 12 '22 04:09 alice-i-cecile

Pull request successfully merged into main.

Build succeeded:

bors[bot] avatar Sep 12 '22 04:09 bors[bot]