`get_unchecked` isn't unchecked

[digama0]

This can be seen in the assembly generated by the get_unchecked function. Although there is no bounds checking on the array, it still checks whether the entry is occupied and so there is a branch and panic handling code due to the use of unreachable!(). I think that logically, both kinds of checking are the same type of error (you accessed an invalid index), so it makes sense for get_unchecked to do no branches at all, only address arithmetic.

The only open question is whether the documentation should be adjusted to use a term other than "bounds checking" here since the set of valid slab indices is not an interval.

[taiki-e]

Could you update MSRV in travis.yml to 1.27.0? (that is needed to fix CI)

https://github.com/carllerche/slab/blob/f1327f72fb14fb387e07a6123300eb7a9da7c4e8/.travis.yml#L8-L9

[taiki-e]

On second thought, I think the addition of safety requirements like this should actually be considered a breaking change.

[hawkw]

Although there is no bounds checking on the array, it still checks whether the entry is occupied and so there is a branch and panic handling code due to the use of unreachable!(). I think that logically, both kinds of checking are the same type of error (you accessed an invalid index), so it makes sense for get_unchecked to do no branches at all, only address arithmetic.

We could elide this check by replacing the enum Entry<T> type with a union, which would allow us to make get_unchecked totally branchless. It could be worth thinking about whether or not this is worth the cost of additional unsafety.

[digama0]

This code (in the PR) has the same performance as the union approach for get_unchecked, but if the implementation actually used union this would affect the ability to have regular get (with bounds checking), and in particular it would make it impossible to do iterator traversals since you don't know what's filled and what isn't. Also you can't deallocate unless you know what's filled. You can probably save space by using a union in the elements of the slab and storing a metadata bitset with the occupied status, although that's a lot of unsafe code.

[taiki-e]

Related: #40, #46

[digama0]

I added the documentation change from https://github.com/tokio-rs/slab/issues/40#issuecomment-468826669 to improve the terminology from "bounds checking". But unlike the author of that comment, I do think this PR is worth merging, even if it is a breaking change requiring a semver bump.

[taiki-e]

If there are other breaking changes, I'm okay with releasing this and them together, but I don't want to bump the major version just for this optimization.

[digama0]

Okay, I'm not the maintainer so do what you think is best. That said, I imagine that breaking changes to a crate like this don't happen very often, so that stance might delay it for a long time.

[taiki-e]

that stance might delay it for a long time.

I agree that this is likely to delay. However, slab is widely used so I want to avoid major version bumps as much as possible.