Make stashes generic over index types #3

spersson · 2016-12-17T07:22:11Z

Inspired by the petgraph library. The reason for a user to choose an index type smaller than usize is probably not to make Entry smaller, since most likely the V type is bigger anyway. The reason is for when storing lots of indices somewhere else, like when building graphs on top of stash. In that case the graph can end up storing lots of indices, so that size may actually matter.
So, one option could have been to keep the next_free field as an usize. I chose not to do that, thinking why not go all the way.
No testing done except from seeing that it compiles on rust 1.13.

Stebalien · 2016-12-29T23:36:17Z

After thinking about this for a bit, I've decided not to merge as this adds a fair amount of complexity for little gain. Instead, I can guarantee that a stash will "pack" efficiently. This means that if you never insert more than n elements in a stash, all indices will be less than n. This means that, for example, if you store at most 256 elements, you always safely truncate the index to a u8.

If you disagree, feel free to discuss.

Stebalien · 2017-02-23T03:56:16Z

So, this came up on #4 and I'm now interested in a simplified version. Specifically, would you be interested in modifying this to work with the following Index trait?

// We need this trait to allow panics when the index is out of range.
pub trait Index {
    // Panics when `idx` is out of range.
    fn from_usize(idx: usize) -> Self;
    fn into_usize(self) -> usize;
}

// Auto implement this for types equivalent to `usize`.
impl<T> Index for T where T: From<usize> + Into<usize> {
    fn from_usize(idx: usize) -> Self {
        From::from(idx)
    }
    fn into_usize(self) -> usize {
        Into::into(self)
    }
}

This should simplify this patch significantly.

spersson · 2017-02-23T13:24:03Z

I might have a bit of time now yes. But I'm unclear on what implementation you have in mind... Because you say that the changes needed would be much less? I really don't see much difference from my proposed changes. The only difference with your trait is dropping the max() function and a few convenience methods. (which is fine with me, those methods could always be added to a newtype index wrapper if needed).
The blanket impl by depending on From and Into traits seems like a great idea!

Stebalien · 2017-02-23T16:53:38Z

Because you say that the changes needed would be much less?

The way I'm envisioning it, you'd convert to a usize at the beginning of the function and back to an Ix: Index at the end. Internally, you'd store and use usize's.

I'd also hold off on the implementation for UniqueStash for now (it doesn't help in your case anyways
as Tag will be quite large due to the version field).

spersson · 2017-02-24T13:49:11Z

I have made changes that compiles without problem now. But I'm a bit stuck on the type inference not working in the tests and examples anymore. I've updated my fork with what I have so far. I will continue looking into what can be done about the current problem, but if you have any advice it would be welcome! And any other comments of course....

spersson · 2017-02-24T14:20:47Z

I found this post to be informative: http://stackoverflow.com/questions/37748306/default-generic-type-parameter-cannot-be-inferred
So it seems the options are

Accept the loss of ergonomics, user needs to supply type when constructing a Stash. (It's actually the V parameter that is needed now, not Ix... weird.)
Go for the other proposed solution of a separate "default_new" method for constructing an instance using default index type.
Don't make index type a type parameter of the Stash struct, only for the get, take, put methods. Would make it possible to have different index types in calls to get and put on the same stash, which seems not desirable. And I don't know if the current "impl trait" feature would allow us to do this, needed for the put method.
scrap the whole idea.
??

Stebalien

Looking good, just a few nits.

Stebalien · 2017-02-24T17:42:28Z

src/stash/mod.rs

    inner: iter::Enumerate<slice::Iter<'a, Entry<V>>>,
    len: usize,
+    _marker: marker::PhantomData<Ix>,


Technically, this should be marker::PhantomData<fn() -> Ix> to indicate that we don't actually store any Ixs although I'm not sure if it makes any difference...

Stebalien · 2017-02-24T17:45:09Z

src/stash/mod.rs

    data: Vec<Entry<V>>,
    size: usize,
    next_free: usize,
+    // add a phantom user of the Ix type to make sure an instance of Stash is bound to one
+    // specific index type, separate calls to put and get can't use different index types.
+    _marker: marker::PhantomData<Ix>,


Shoudl probably be marker::PhantomData<fn(Ix) -> Ix>.

Stebalien · 2017-02-24T17:47:32Z

src/stash/mod.rs

@@ -268,7 +279,7 @@ impl<V> Stash<V> {
    ///
    /// *Panics* if the size of the `Stash<V>` would overflow `usize::MAX`.
    #[inline]
-    pub fn put(&mut self, value: V) -> usize {
+    pub fn put(&mut self, value: V) -> Ix {
        let loc = self.next_free;


I'd prefer to create the index here (to panic early if necessary): let idx = Ix::from_usize(self.next_free)

Stebalien · 2017-02-24T17:49:36Z

src/index.rs

@@ -0,0 +1,18 @@
+// We need this trait to allow panics when the index is out of range.


Should be documented (even if it's just a short "this is the index trait (auto implemented by usize)...").

Stebalien · 2017-02-24T17:56:41Z

But I'm a bit stuck on the type inference not working in the tests and examples anymore.

That's... annoying. However, I'd still like this feature.

So it seems the options are

I'm not a fan of 3 and 4. I'm considering a variant of 2 where new and with_capacity use the default (usize) index and introduce additional methods for custom index types but I'll have to think on it a bit.

Adds the Index trait and makes Stash generic over any type that implements that trait. It still stores and works with usize internally, only convert indices at the API surface. The Stash::new() method is the only method that is not made generic over the index type used, it constructs a Stash with usize indices. It was done that way to keep examples and simple usages minimal, without need for type annotations, same as before this patch.

spersson · 2017-02-25T07:04:35Z

Ok, I've updated the commit according to your comments and also changed the new() method to only be available for creating Stash with usize index. For creating Stashes with other index types the default() method can be used. This is what petgraph is doing. I also did some minor updates to documentation comments. I'm pretty happy about how it looks now. Confirmed no regression on the benchmarks also.

Stebalien · 2017-02-25T21:22:34Z

Thanks.

To avoid breaking changes, how do you feel about treating with_capacity as a convenience method as well? If you need to specify the index type, you can always write:

let mut stash: Stash<T, usize> = Stash::default();
stash.reserve(capacity);

Meaning that it will create a Stash that uses usize as index type. In the general case where a custom index type might be needed the user can construct the Stash with Stash::default() and then call reserve() to allocate needed capacity. This way we don't break existing code and the new use case of custom index types is still possible (and also pretty convenient.. not bad.) Also add a new example to show how a custom index type can be constructed and used.

Stebalien · 2017-02-26T17:27:20Z

Awesome! Thanks!

Stebalien closed this Dec 29, 2016

Stebalien mentioned this pull request Feb 22, 2017

Add debug checks for misusage of Stash indices #6

Open

Stebalien reopened this Feb 23, 2017

spersson force-pushed the master branch from c9c79ae to 234a537 Compare February 24, 2017 13:46

Stebalien requested changes Feb 24, 2017

View reviewed changes

spersson force-pushed the master branch from 234a537 to f80d0dd Compare February 25, 2017 06:30

Stebalien approved these changes Feb 26, 2017

View reviewed changes

Stebalien merged commit 8b5ca7b into Stebalien:master Feb 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make stashes generic over index types #3

Make stashes generic over index types #3

spersson commented Dec 17, 2016

Stebalien commented Dec 29, 2016

Stebalien commented Feb 23, 2017

spersson commented Feb 23, 2017

Stebalien commented Feb 23, 2017

spersson commented Feb 24, 2017

spersson commented Feb 24, 2017 •

edited

Loading

Stebalien left a comment

Stebalien Feb 24, 2017

Stebalien Feb 24, 2017

Stebalien Feb 24, 2017

Stebalien Feb 24, 2017

Stebalien commented Feb 24, 2017

spersson commented Feb 25, 2017

Stebalien commented Feb 25, 2017

Stebalien commented Feb 26, 2017

		@@ -0,0 +1,18 @@
		// We need this trait to allow panics when the index is out of range.

Make stashes generic over index types #3

Make stashes generic over index types #3

Conversation

spersson commented Dec 17, 2016

Stebalien commented Dec 29, 2016

Stebalien commented Feb 23, 2017

spersson commented Feb 23, 2017

Stebalien commented Feb 23, 2017

spersson commented Feb 24, 2017

spersson commented Feb 24, 2017 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Feb 24, 2017

Choose a reason for hiding this comment

Stebalien Feb 24, 2017

Choose a reason for hiding this comment

Stebalien Feb 24, 2017

Choose a reason for hiding this comment

Stebalien Feb 24, 2017

Choose a reason for hiding this comment

Stebalien commented Feb 24, 2017

spersson commented Feb 25, 2017

Stebalien commented Feb 25, 2017

Stebalien commented Feb 26, 2017

spersson commented Feb 24, 2017 •

edited

Loading