Add compact() and optimize recreate_vacant_list()

[tormol]

The first commit makes shrink_to_fit() remove any vacant entries after the last occupied one,
so that the Vec can shrink below the maximum size of the slab.
This usually requires iterating through all remaining entries afterwards to repair the vacant list, which takes O(n) time. I think this is acceptable as shrinking the allocation might involve copying all elements, so callers must already be prepared for that.
(Fixes #38)

The second commit adds a .compact() method which moves elements before shrinking.
It takes an |element, old_key, new_key|->Result closure which is called before moving an element.
This is designed for use with mio's reregister() and supports aborting if changing the key for one value.
The proposed API isn't particularly ergonomic, as it requires explicitly specifying an error type for the closure if it never returns Err.
I initially tried to implement this with an iterator interface, but that is not possible since returned references must be valid for the lifetime of the iterator.
Because .remove() re-inserts vacant entries before panicking, I've made sure that the vacant list is repaired if the closure unwinds. Dropping that requirement would simplify the code quite a bit though.

[carllerche]

The shrink_to_fit change looks good to me, I would be happy to merge it as is.

Re: the compact() change, I'm thinking an iterator based API would be more idiomatic. I'm drawing from drain() for precedence.

What do you think?

Maybe we should split the two features into separate PRs to avoid holding up shrink_to_fit?

[tormol]

An iterator that produces &mut Ts and doesn't allocate appears to be impossible, due to the requirement of the iterator trait that any returned reference must be valid for the lifetime of the iterator.
Not returning mutable references I think would prevent some use cases.
Returning a struct that doesn't implement Iterator but has an inherent next() would be easy to implement, but not as nice as iterator if one doesn't need cancellation, and less convenient than a closure when one does.

Supporting canceling moves with an iterator interface also isn't straightforward:
My first iterator attempt took next() being called as confirmation that the previous returned value should be moved. This meant that calling next() and getting None was different from not calling it. (so using .take() or .take_while() would lead to surprising behavior.
My second attempt used inherent methods, but then one has to break out of the loop to call them.

You can cherry-pick the shrink_to_fit() commit, I pushed again because I found an optimization for recreate_vacant_list() while working on the second iterator attempt.

I'll change the closure to return bool instead - if somebody does want to move out a value they can put it in a captured Option variable, and that works in the Ok case too.

[tormol]

Why an Iterator(that supports canceling or shrinks afterwards) is impossible (without unsafe code):

My first iterator attempt didn't compile because references produced by an iterator must be valid for the entire lifetime of the iterator.

Drain cannot work for the same reason: It returns by value and therefore moves it.

Your comment gave me the idea of delaying moves until Drop, which would require iterating entries twice but should keep the references valid long enough. That doesn't compile either (without unsafe), because the compiler cannot verify that the entry pointed to by a previously returned reference isn't accessed again.

The iterator cannot use slice::IterMut because it also needs to store a reference to the vector or slab for moving the elements afterwards, which would alias the slice iterator.

Swapping self.entries with an empty vector and storing it in the iterator would break the aliasing, but that makes the slice iterator becomes self-referential...

What we need is an iterator that does the opposite of Drain: produces &mut T while owning the Vec and that can be turned back into the vector.

When I mentioned allocating in the previous comment I was thinking that moving all values to a new vector would allow the iterator to use IntoIterator or Drain, but they are incompatible for the reason explained above.
I tried using Rc<RefCell<&mut Slab>, but while cell::RefMut can be mapped to another type (IterMut), the RefCell cannot, and the cell::RefMut needs a lifetime to borrow.

[tormol]

Updated documentation to reflect returning bool instead of Result.

I've actually gotten an Iterator-producing version to work, but it doesn't support undoing moves of previously returned values. It does not reduce capacity or entries.len() either, so one must call shrink_to_fit() afterwards.

As calling this is not something one will do in many places, I don't think ergonomics is important, and I would rather have the closure version which supports more scenarios.

[carllerche]

@tormol Ok, thanks for doing the exploration. Your reasoning for why an iterator is not possible makes sense. I am 👍 on the change, it just needs to be rebased & the merge conflict sorted out.

[tormol]

(Rebased)