-
Notifications
You must be signed in to change notification settings - Fork 1.1k
MVP no_std for wgpu-core
#7746
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
base: trunk
Are you sure you want to change the base?
MVP no_std for wgpu-core
#7746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really well documented both in code and PR description, thank you!
Would like to see some of it paraphrased in the feature docs to help users make the right choice.
Make once_cell optional by falling back to core::cell:OnceCell.
a. Note that once_cell is no_std compatible, but it requires activating the critical-section feature to access its implementation of OnceCell. Adding such a feature would therefore add critical-section to the lockfile, requiring thorough review due to its use of unsafe and extern "Rust". I believe it is a safe inclusion, but it can be avoided here so I'd rather save that quality of life feature for a follow-up. Users are still able to use once_cell with wgpu-core on no_std, they just have to enable once_cell/critical-section themselves.
b. Note that core::cell::OnceCell is !Sync, so users should prefer to enable critical-section themselves.
It irks me to add yet another feature just because of that, also given that once_cell is such a small and common crate. I think if we could we'd want to imply not enabling std to enable once_cell/critical-section.
Since this isn't possible I'd instead argue it's better to just keep always depending on once_cell and point out to users that they have to use once_cell/critical-section if they want Sync. That's already the case with the PR as-is, only that a user also has to enable the once_cell feature.
This would save us feature permutations, an extra code path and complexity for users. What do you think?
Agreed! The ideal solution (that we can implement without changes to Cargo or |
|
I've split out the |
|
Sorry for the long lead times on these. It's been quite busy at work and I'm now out for a couple of days. Will likely get to it some point next week 🤞 |
|
No stress! Thanks for your help 🙂 |
|
finally getting coming back to this after a break! First of all, thanks for all the added docs, looks great now.
Not entirely sure I'm following it all the way: right now the PR exposes But either way we still end up with imho way to many hard to use feature knobs. I'm wondering if we instead should adjust a little bit:
@ErichDonGubler does Mozilla care about dependencies that show up in the lock file but aren't ever enabled in Firefox? Should be fine, after all we also have testing dependencies in the same situation, no? |
So long as they don't show up in |
|
great! :) @bushrat011899 sounds like we could above mentioned suggestion then, what do you think?
|
| let guard = loop { | ||
| if let Ok(guard) = self.0.try_borrow() { | ||
| break guard; | ||
| } | ||
| core::hint::spin_loop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will definitionally never yield as there isn't another thread that can access the RefCell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i'd prefer if not having either spin or parking_lot was a compile error. Similarly, do we need to have separate std and parking_lot features?
| // NOTE: Unlike `once_cell`, the `OnceCell` from `core` is _not_ `Sync`. | ||
| use core::cell::OnceCell; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use once_cell::unsync for these?
| Ok(weak) | ||
| })?; | ||
| }; | ||
| cfg_if::cfg_if! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pains me a lot to have yet another layer of stuff to try to get through. I'm worried about the consequences of various program orderings in the non-once-cell branch here, so I'll need to come back and re-audit this.
|
@bushrat011899 poke! |
|
Hey! I know personal life got busy for you, take your time, but I'm going to demote these two PRs to drafts, and you can re-promote them when they're ready for review again! |
Connections
Description
Adds
no_stdsupport towgpu-corethrough 3 changes:num-traitswithintimestamp_normalizationto account for the floating point methods only available withstd.parking_lotoptional by falling back to eitherstd::sync::{Mutex, RwLock},spin::{Mutex, RwLock}, orcore::cell::RefCellbased on selecting eitherparking_lot,std,spin, or no locking implementation.a. Note that
spinwas already in the lockfile viacrossbeam-dequeso this feature is "free"b. Note that
RefCellis!Sync, so users should selectspininno_stdenvironments. The use ofRefCellas a fallback just allows compilation to succeed without any features enabled.once_celloptional by falling back tocore::cell:OnceCell.a. Note that
once_cellisno_stdcompatible, but it requires activating thecritical-sectionfeature to access its implementation ofOnceCell. Adding such a feature would therefore addcritical-sectionto the lockfile, requiring thorough review due to its use ofunsafeandextern "Rust". I believe it is a safe inclusion, but it can be avoided here so I'd rather save that quality of life feature for a follow-up. Users are still able to useonce_cellwithwgpu-coreonno_std, they just have to enableonce_cell/critical-sectionthemselves.b. Note that
core::cell::OnceCellis!Sync, so users should prefer to enablecritical-sectionthemselves.To preserve existing behavior, the newly added
parking_lotandonce_cellfeatures are enabled by default.Testing
Added
wgpu-coreto thewasm32v1-noneCI test.Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.