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

Make the crate no_std + alloc #44

Closed
wants to merge 26 commits into from
Closed

Conversation

GarkGarcia
Copy link
Contributor

This is far from complete, but I went ahead and replaced std for core in all of the easy places. I also created the std feature and configured all relevant functions to only be exposed when using it.

I'd like some input before I proceed.

@GarkGarcia GarkGarcia mentioned this pull request Jan 29, 2020
src/map.rs Outdated Show resolved Hide resolved
@GarkGarcia
Copy link
Contributor Author

On regards to your point about having to figure out what to do with the default value of S, apparently the compiler is happy with S = RandomState, even though it is not defined in a no_std environment 🤷. I haven't tried compiling it with no_std though.

src/map.rs Outdated Show resolved Hide resolved
src/map.rs Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated Show resolved Hide resolved
Pablo Escobar Gaviria added 3 commits January 29, 2020 20:26
… `RandomState`.

Now the `S` type parameter is only required to implement `Default`.
@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

I think we're still missing a couple of important things in src/lib.rs. crossbeam-epoch/src/lib.rs is probably good inspiration. In particular, we'll need to add cfg_if to Cargo.toml, and then something like

#![cfg_attr(not(feature = "std"), no_std)]

#[macro_use]
extern crate cfg_if;
#[cfg(feature = "std")]
extern crate core;

cfg_if! {
    if #[cfg(feature = "alloc")] {
        extern crate alloc;
    } else if #[cfg(feature = "std")] {
        extern crate std as alloc;
    }
}

Copy link
Contributor Author

@GarkGarcia GarkGarcia left a comment

Choose a reason for hiding this comment

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

I think we're still missing a couple of important things in src/lib.rs. crossbeam-epoch/src/lib.rs is probably good inspiration. In particular, we'll need to add cfg_if to Cargo.toml, and then something like

#![cfg_attr(not(feature = "std"), no_std)]

#[macro_use]
extern crate cfg_if;
#[cfg(feature = "std")]
extern crate core;

cfg_if! {
    if #[cfg(feature = "alloc")] {
        extern crate alloc;
    } else if #[cfg(feature = "std")] {
        extern crate std as alloc;
    }
}

Alright. Are extern crate alloc and extern crate core necessary thought? The compiler is complaining about the fact that they are unused...

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

Ah, no, you're right, with the 2018 edition you may be able to remove them. The one thing you will need somewhere is probably this though

#[cfg(feature = "std")]
use std as alloc;

But could very well be that we can get rid of cfg-if entirely!

@GarkGarcia
Copy link
Contributor Author

GarkGarcia commented Jan 29, 2020

Ah, no, you're right, with the 2018 edition you may be able to remove them. The one thing you will need somewhere is probably this though

#[cfg(feature = "std")]
use std as alloc;

But could very well be that we can get rid of cfg-if entirely!

The compiler still complains about use std as alloc being unused. Is this supposed to be placed at lib.rs?

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

Hmm, I was thinking of elsewhere in the crate where we try to use alloc::, but apparently that's just now dealt with automatically? I guess just remove it then? What happens if you try to compile it locally now with --no-default-features?

@GarkGarcia
Copy link
Contributor Author

Hmm, I was thinking of elsewhere in the crate where we try to use alloc::, but apparently that's just now dealt with automatically? I guess just remove it then? What happens if you try to compile it locally now with --no-default-features?

I get a bunch of errors related to the absence of some things auto-imported from std. Also, we're missing epoch::pin in a bunch of places.

I'll work on the auto-imported things. I don't see a clear path on how to deal with epoch::pin though. If we were to simply enforce cfg(feature = "std") the API would get a bit unusable in a no_std context...

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

I think anything where we take a &epoch::Guard is fine in no_std, but anything where we have to construct a epoch::Guard ourselves is a no-go in that environment (sadly).

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

I am very surprised that CI doesn't catch that it doesn't compile with --no-default-features. That should certainly cause a CI error.

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

It looks like it doesn't even re-compile when --no-default-features is passed..

@GarkGarcia
Copy link
Contributor Author

It looks like it doesn't even re-compile when --no-default-features is passed..

Yep.

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

If you do the following locally, in order, do you get any errors?

$ cargo check --all --bins --examples
$ cargo check --all --bins --examples --no-default-features

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

As in, just do:

#![cfg_attr(feature = "std", deny(missing_debug_implementations))]

(and remove the existing deny(missing_debug_implementations))

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Another alternative is to have a separate impl on no_std that just prints HashMap.

@Amanieu
Copy link

Amanieu commented Jan 30, 2020

Actually there is a potential source of unsoundness here: crossbeam_epoch allows you to create separate Collector instances aside from the global one used by epoch::pin. However you need to make sure that all Guards used all come from the same Collector.

The approach I went with for crossbeam-skiplist is that the SkipList type is the low-level interface which must be associated with a Collector when created and must have a Guard passed in for every operation. The SkipMap type is the high level interface which hides all the epoch-related stuff from you, but cannot be used in no_std mode.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

@Amanieu oh, that is interesting. I believe that's a problem beyond just no_std though -- it applies to all the methods we provide that take a &Guard? @cuviper this suggests to me that we want to "invert" what you proposed in #45, and follow a design like the one Amanieu proposed above where the low level takes a Collector, and the high level manages the Collector for you.

Doesn't that mean that users cannot take advantage of the global epoch collection though? As in, they can't share the collection between many different programs that use epoch::pin.

@Amanieu
Copy link

Amanieu commented Jan 30, 2020

The high-level wrapper can use the default collector (the one used by epoch::pin):

    pub fn new() -> SkipMap<K, V> {
        SkipMap {
            inner: base::SkipList::new(epoch::default_collector().clone()),
        }
    }

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

That does mean that methods cannot generally return references though, right? Since you would not be able to give the return value an appropriate lifetime, as it would be tied to the guard we construct inside each method. That's pretty unfortunate.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

That is, in the high-level type. In the low-level type you can still take a &Guard and just assert that the collector matches (like it looks as though you're doing in SkipList)

@cuviper
Copy link
Collaborator

cuviper commented Jan 30, 2020

A newtype Guard could be enforced to only be constructed the way you want, e.g. only global epoch::pin(). Not sure what that would be on no_std though.

Two of the motivations for #45 are Index and IntoIterator for &HashMap, which can't work with implicit temporary guards.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Actually, I have a proposal. Writing it up as an issue as we speak. Will link when written.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Okay, for the unsoundness let's continue that conversation in #46. I think this PR is mostly orthogonal to that issue.

@GarkGarcia for this PR, I think the main remaining thing is to merge master

@GarkGarcia
Copy link
Contributor Author

@GarkGarcia for this PR, I think the main remaining thing is to merge master

Could you handle that for me please?

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Okay, so, I think the next (last?) challenge is that parking_lot does not support no_std. We may have to be generic over the type of lock to use...

This is necessary since there is no particular lock implementation that
"makes sense" on `no_std`. This patch changes all types to be generic
over any lock type that implements `lock_api::RawMutex`, and defaults to
`parking_lot::RawMutex` on `std`.

Note that this patch forks the struct definition for `HashMap` into one
for `std` (which has a default for `L`) and one for `no_std` (which does
not). This is fine since any code written _without_ the `std` feature
_will_ compile with the `std` feature enabled (`indexmap` does the same;
the reverse is not true). We _could_ introduce an intermediate private
struct to avoid repeating the definition, but it would require writing
`self.inner` all over the place, which would be sad. This seemed
marginally cleaner.

Also, this diff makes me want implied bounds a lot:
rust-lang/rust#44491
@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

All right, I fixed that by making us generic over lock_api::RawMutex. It ain't pretty, but it works. See also the commit message on adb58e0 for details. This introduced another "default only on std" type parameter, where I opted to go for the indexmap approach of simply not setting a default without std.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

🎉 tests pass 🎉

I'll merge this in a few hours unless there are any objections. The L parameter is unfortunate, but I don't see how we get around it if we want to support no_std. I think the only question is "is it important is it for us to support no_std for this crate". If the answer is that it seems valuable, then I think this is how it has to be.

Copy link
Collaborator

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

The exposed lock seems like a step too far to me, that perhaps no_std isn't really worth it. The lock should really be an internal detail, not something the user should care about like the hasher. But if you really want to support no_std, I guess there's not much choice...

/// See the [crate-level documentation](index.html) for details.
pub struct HashMap<K, V, S = RandomState> {
#[cfg(feature = "std")]
pub struct HashMap<K, V, L = parking_lot::RawMutex, S = RandomState>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest L and S should be reversed, so HashMap<K, V, S> still looks like the standard collection. This would also simplify the test cases where you had to start specifying the lock type to use a different hasher, which feels like an API wart. However, that means you'd have to remove the default S in the no-std configuration too.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, not wanting to remove the default S in no-std is why I ordered them this way. I'm honestly not sure which is better/worse. My inclination is to make things harder for no_std folks when given a trade-off, which would suggest no defaults in no-std and L last.


/// An iterator over a map's entries.
///
/// See [`HashMap::iter`](crate::HashMap::iter) for details.
#[derive(Debug)]
pub struct Iter<'g, K, V> {
pub(crate) node_iter: NodeIter<'g, K, V>,
pub struct Iter<'g, K, V, L>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a default L for all of the iterator types too?

Copy link
Owner

Choose a reason for hiding this comment

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

Bah, then we'd have to duplicate all of these too. One other option here is to define a default value on no_std that panics when used. It ain't great at all, but it would allow us to not duplicate all the structs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that's why hashbrown uses an uninhabited default on no-std.

Copy link

Choose a reason for hiding this comment

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

No, hashbrown uses ahash on no_std. It only uses an uninhabited default when compiled as a dependency for libstd (because the libstd build system is weird and makes dependencies difficult).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok, I misunderstood that context, and hashbrown doesn't need a lock like this. An uninhabited default might still work though, or we could use macro_rules! to hide the duplication.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

I don't disagree with you that the change is becoming pretty painful API-wise. I've put out a call for input from the people who actually use no_std to see if we can make a more informed decision about whether the pain here is worth it.

If we decide it's not worth it, then I still think there are a bunch of changes from this PR we may want to adopt in master (like the explicit Guard passing in many places).

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Let's continue this in #48. I've extracted the changes unrelated to no_std to #47.

@jonhoo jonhoo closed this Jan 30, 2020
jonhoo added a commit that referenced this pull request Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants