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

Parametrize RawTable, HashSet and HashMap over an allocator #133

Merged
merged 16 commits into from
Oct 25, 2020

Conversation

hansihe
Copy link
Contributor

@hansihe hansihe commented Dec 9, 2019

This change would make the RawTable usable with things like arena allocation.

  • RawTable has a new type parameter, A: Alloc + Clone
  • When the nightly flag is passed, Alloc will be the trait in
    alloc::Alloc.
  • On stable, a minimal shim implementation is provided, along with an
    implementation for the global allocator.
  • No public APIs changed.
  • For HashMap, everything is monomorphized to the global allocator,
    and there should be no performance or size overhead.

* `RawTable` has a new type parameter, `A: Alloc + Clone`
* When the `nightly` flag is passed, `Alloc` will be the trait in
`alloc::Alloc`.
* On stable, a minimal shim implementation is provided, along with an
implementation for the global allocator.
* No public APIs changed.
* For `HashMap`, everything is monomorphized to the global allocator,
and there should be no performance or size overhead.
src/raw/alloc.rs Outdated Show resolved Hide resolved
src/raw/mod.rs Outdated Show resolved Hide resolved
* Change order of type parameters
* Handle null case for `alloc`
* Run rustfmt
@hansihe
Copy link
Contributor Author

hansihe commented Dec 9, 2019

Ah, and I didn't test the rayon feature. I'll do that too.

@hansihe
Copy link
Contributor Author

hansihe commented Dec 9, 2019

Pretty sure the last errors are not my fault

@Amanieu
Copy link
Member

Amanieu commented Dec 9, 2019

Yea we're having CI issues due to cross-rs/cross#357. I will merge as soon as this is resolved.

@hansihe
Copy link
Contributor Author

hansihe commented Jan 3, 2020

@Amanieu Regarding your reply in fitzgen/bumpalo#54

I'm happy to accept a PR adding allocator_api support to hashbrown. You can extend your existing PR (#133) to add allocator support to the full HashMap API instead of just the internal RawTable API.

Wouldn't this cause problems with API stability? Or were you thinking behind a feature flag?

@Amanieu
Copy link
Member

Amanieu commented Jan 3, 2020

I'll just make a major version bump, it's fine.

@hansihe
Copy link
Contributor Author

hansihe commented Feb 15, 2020

Just to add some context as to why this hasn't moved anywhere on my end.

I am a bit unsure how to handle things like HashSet::union.

    pub fn union<'a>(&'a self, other: &'a HashSet<T, S>) -> Union<'a, T, S> {
        let (smaller, larger) = if self.len() <= other.len() {
            (self, other)
        } else {
            (other, self)
        };
        Union {
            iter: larger.iter().chain(smaller.difference(larger)),
        }
    }

This function takes two sets, which we presumably want the ability to be from two different allocators. Internally it does a compare, and returns a Union struct containing a reference to the largest of the two sets, along with the difference with the other set.

If the two sets are from different allocators, this approach wouldn't work as the returned struct would have a different allocator type parameter depending on which set is larger.

I see a couple of different options:

  1. Just don't implement problematic functions for cases where there are two different allocators. This seems unfortunate to me.
  2. Make Union an enum instead of a struct, and have one variant for each of the cases. This exposes both allocators as type parameters of the enum. I really don't like this.
  3. Make the function as is today work as in 1., but have a separate one that calculates the union into a whole new Set. This seems unfortunate to me as well, but it might be the best option.

Thoughts?

@Amanieu
Copy link
Member

Amanieu commented Feb 15, 2020

I think for now it would be best to stick to the simplest solution, which is to require both HashSet to have the same allocator type.

@hansihe
Copy link
Contributor Author

hansihe commented Feb 28, 2020

Again, I don't think the last error is due to my changes

@hansihe hansihe changed the title Parametrize RawTable over an allocator Parametrize RawTable, HashSet and HashMap over an allocator Mar 5, 2020
@hansihe
Copy link
Contributor Author

hansihe commented Mar 5, 2020

Seems like the allocator trait (AllocRef right now) is under major flux right now in the latest nightlies. I'll update to latest, but I guess it might make sense to wait with merging until things settle a bit?

@Amanieu
Copy link
Member

Amanieu commented Mar 5, 2020

Yea we've got a few changes lined up. Ideally I would like to wait until AllocRef is stable before merging this, but this may take too long if you need this feature right now (e.g. for use with bumpalo).

@hansihe
Copy link
Contributor Author

hansihe commented Mar 5, 2020

I'm fine with using my forks for now.

Would you mind dropping a comment when you believe most changes are out of the way and I'll update to latest?

@Amanieu
Copy link
Member

Amanieu commented Mar 5, 2020

Sure! Will do.

This was referenced Mar 15, 2021
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