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

adding no_std support #59

Closed
wants to merge 4 commits into from
Closed

adding no_std support #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2842aad
adding `no_std` support
dscso May 2, 2024
caac55c
using std rwlock and spin on no_std environments
dscso May 9, 2024
3ab0185
fmt
dscso May 9, 2024
fb7ce48
remove conditional double import
dscso May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,16 @@ harness = false
name = "broadcast_bench"

[features]
default = ["std"]
std = ["event-listener/std", "event-listener-strategy/std"]


[dependencies]
event-listener = "5.0.0"
event-listener-strategy = "0.5.0"
futures-core = "0.3.21"
event-listener = { version = "5.0.0", default-features = false }
event-listener-strategy = { version = "0.5.0", default-features = false }
futures-core = { version = "0.3.21", default-features = false }
pin-project-lite = "0.2.13"
spin = { version = "0.9.8" }
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use this unconditionally, even with std?

Copy link
Author

Choose a reason for hiding this comment

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

Aaaactually this is a very good note! I was way too fast in replacing this. I found this in another implementation of channels, but there are some significant downsides. I could change the implementation to only use spin in no_std environments... Unfortunately that means I have to do this quite a lot:

#[cfg(feature = "std")]
return self.inner.read().unwrap().queue.len();
#[cfg(not(feature = "std"))]
return self.inner.read().queue.len();

Alternatively I could also use parking_lot as drop in replacement for std::rwlock since it should be faster as well.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

#[cfg(feature = "std")]
return self.inner.read().unwrap().queue.len();
#[cfg(not(feature = "std"))]
return self.inner.read().queue.len();

You can abstract this away so the #cfg is only done in a few places. i-e implement wrapper type for RwLock that only provides the few methods we use.

Alternatively I could also use parking_lot as drop in replacement for std::rwlock since it should be faster as well.
What do you think?

Let's consider that if we bump into any issues with my suggestion for a simple abstraction type. :)

Copy link
Author

@dscso dscso May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just did a horrible thing:

#[cfg(not(feature = "std"))]
use spin::RwLock as Orig_RwLock;
#[cfg(not(feature = "std"))]
use spin::{RwLockWriteGuard, RwLockReadGuard};

#[cfg(feature = "std")]
use std::sync::RwLock as Orig_RwLock;
#[cfg(feature = "std")]
use std::sync::{RwLockWriteGuard, RwLockReadGuard};


#[derive(Debug)]
pub(crate) struct RwLock<T> {
    rwlock: Orig_RwLock<T>
}

impl<T> RwLock<T> {
    #[inline]
    pub(crate) const fn new(t: T) -> RwLock<T> {
        return RwLock {
            rwlock: Orig_RwLock::new(t),
        }
    }
    #[inline]
    pub(crate) fn read(&self) -> RwLockReadGuard<'_, T> {
        #[cfg(feature = "std")]
        return self.rwlock.read().unwrap();
        #[cfg(not(feature = "std"))]
        return self.rwlock.read();
    }
    #[inline]
    pub(crate) fn write(&self) -> RwLockWriteGuard<'_, T> {
        #[cfg(feature = "std")]
        return self.rwlock.write().unwrap();
        #[cfg(not(feature = "std"))]
        return self.rwlock.write();
    }
}

Is this what u ment? If yes, I'd commit. The only problem I could not solve yet is that there is no need to compile spin if running with std feature enabled. Do you know how do disable a dependency if a feature is not selected?

Copy link
Member

Choose a reason for hiding this comment

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

Is this what u ment?

I did, yes. However,

  • I'd suggest not using any imports but rather using full paths to keep complexity to minumim.
  • don't forget to format it (empty lines between methods).

The only problem I could not solve yet is that there is no need to compile spin if running with std feature enabled. Do you know how do disable a dependency if a feature is not selected?

Hmm.. actually no and I think it's not possible. :( Cargo features are designed to be additive only.

Now we could consider parking_lot but:

  1. It's not faster than std's locks anymore. We actually use to use it and it got removed in Use std RwLock instead of parking_lot. #38.
  2. In parking_lot docs, I don't see any mention of nostd, nor std feature listed there.

Copy link
Member

Choose a reason for hiding this comment

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

Ok perfect I fixed the remaining issues

Not sure I follow. What's the solution for only depending on spin when !std?

Copy link
Author

Choose a reason for hiding this comment

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

Actually I've read this article.

I think the optimal solution would be to use spin for everything except std, wasm and single core no interrupt cores. Here it should check whether the lock is already locked and if so panic.
But as I see here, in concurrent-queue they also use spinning in no_std environment.

I think the main problem emerges if you use a spin lock like this:

lock.read()
{
    lock.write()
}

Then we would have a deadlock.
But since this is not happening, a spin lock should be ok for this library's internal usage in a no_std environment.

Copy link
Member

Choose a reason for hiding this comment

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

I think the optimal solution would be to use spin for everything except std

Sure but I'm talking about the dependency issue. How do we not have a dependency on spin when std is enabled?

lock.read()
{
    lock.write()
}

This is a very usual footgun with locks. I don't think we need to do anything smart about this except ensure that we don't do this. Our locks are internal so we don't need to care about our users having such issues.

Copy link
Author

Choose a reason for hiding this comment

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

I think there is no solution to this honestly. we could add a feature no_std that enables spin but it is not how other libraries handle it.
Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is no solution to this honestly. we could add a feature no_std that enables spin but it is not how other libraries handle it.

That's something I'd be interested in. How are other libs doing this?

Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

Looks like spin has a std feature:

std enables support for thread yielding instead of spinning.

If I understand this correctly, we can use spin but enable std feature when std is enabled on async-broadcast? If so, this would also eliminate the need for the abstractions.

Another option would be to copy paste spin code into this crate and therefore getting rid of the dependency of spin. This could reduce compile time, but is not a really elegant solution in my opinion.

Yeah but if we can't come up with another solution, I'd still consider it as it's preferable to depending on an a completely unused crate. We'll only want to import the bits we need though so it could be a not-too-bad solution.


[dev-dependencies]
criterion = "0.3.5"
Expand Down
Loading
Loading