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

[Merged by Bors] - Remove Sync bound from Local #5483

Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - Remove Sync bound from Local #5483

wants to merge 7 commits into from

Conversation

PROMETHIA-27
Copy link
Contributor

@PROMETHIA-27 PROMETHIA-27 commented Jul 29, 2022 •
edited
Loading

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 may have to be changed, but this is unlikely.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 29, 2022
@alice-i-cecile alice-i-cecile requested a review from BoxyUwU July 29, 2022 14:52
@mockersf
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 29, 2022
@bors
Try #5483:
e815da6
@bors
Copy link
Contributor

bors bot commented Jul 29, 2022

try

Build succeeded:

@harudagondi harudagondi mentioned this pull request Jul 30, 2022
Open
3 tasks
joseph-gio
joseph-gio reviewed Aug 16, 2022
View reviewed changes
Copy link
Member

@joseph-gio joseph-gio left a comment

Choose a reason for hiding this comment

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

I like this a lot. Even though the use-case seems niche, it'll feel really good for that niche user when they realize it just works.

crates/bevy_utils/src/synccell.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/synccell.rs Show resolved Hide resolved
@hymm
Copy link
Contributor

hymm commented Sep 1, 2022

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.

@PROMETHIA-27
Copy link
Contributor Author

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.

@hymm
Copy link
Contributor

hymm commented Sep 1, 2022

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

@PROMETHIA-27
Copy link
Contributor Author

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.

@harudagondi
Copy link
Member

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.

james7132
james7132 approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

One note about a followup to this PR, otherwise LGTM.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 12, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 12, 2022
@PROMETHIA-27
Remove Sync bound from Local (#5483)
05afbc6 
# 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` may have to be changed, but this is unlikely.

Co-authored-by: PROMETHIA-27 <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 12, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Remove Sync bound from Local [Merged by Bors] - Remove Sync bound from Local Sep 12, 2022
@bors bors bot closed this Sep 12, 2022
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
@PROMETHIA-27 @nicopap
Remove Sync bound from Local (bevyengine#5483)
d144763 
# 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` may have to be changed, but this is unlikely.

Co-authored-by: PROMETHIA-27 <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@PROMETHIA-27 @james7132
Remove Sync bound from Local (bevyengine#5483)
83ba050 
# 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` may have to be changed, but this is unlikely.

Co-authored-by: PROMETHIA-27 <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@PROMETHIA-27 @ItsDoot
Remove Sync bound from Local (bevyengine#5483)
0dd7809 
# 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` may have to be changed, but this is unlikely.

Co-authored-by: PROMETHIA-27 <[email protected]>
@joseph-gio joseph-gio mentioned this pull request Feb 16, 2023
Closed
bors bot pushed a commit that referenced this pull request Feb 17, 2023
@joseph-gio
Allow shared access to SyncCell for types that are already Sync (#…
38568cc 
…7718)

# Objective

The type `SyncCell<T>` (added in #5483) is used to force any wrapped type to be `Sync`, by only allowing exclusive access to the wrapped value. This restriction is unnecessary for types which are already `Sync`.

---

## Changelog

+ Added the method `read` to `SyncCell`, which allows shared access to values that already implement the `Sync` trait.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 18, 2023
@joseph-gio @myreprise1
Allow shared access to SyncCell for types that are already Sync (b…
ce3068a 
…evyengine#7718)

# Objective

The type `SyncCell<T>` (added in bevyengine#5483) is used to force any wrapped type to be `Sync`, by only allowing exclusive access to the wrapped value. This restriction is unnecessary for types which are already `Sync`.

---

## Changelog

+ Added the method `read` to `SyncCell`, which allows shared access to values that already implement the `Sync` trait.
@joseph-gio joseph-gio mentioned this pull request Aug 26, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2023
@joseph-gio
Relax more Sync bounds on Local (#9589)
da8ab16 
# Objective

#5483 allows for the creation of non-`Sync` locals. However, it's not
actually possible to use these types as there is a `Sync` bound on the
`Deref` impls.

## Solution

Remove the unnecessary bounds.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@joseph-gio
Relax more Sync bounds on Local (bevyengine#9589)
6ae557f 
# Objective

bevyengine#5483 allows for the creation of non-`Sync` locals. However, it's not
actually possible to use these types as there is a `Sync` bound on the
`Deref` impls.

## Solution

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

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@james7132 james7132 james7132 approved these changes

@joseph-gio joseph-gio joseph-gio approved these changes

@BoxyUwU BoxyUwU Awaiting requested review from BoxyUwU

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@PROMETHIA-27 @mockersf @hymm @harudagondi @alice-i-cecile @james7132 @joseph-gio