Switch to hashbrown's RawTable internally

[cuviper]

This switches IndexMapCore from its bespoke hash table to hashbrown::raw::RawTable<usize>, storing just the index into our ordered entries: Vec<Bucket<K, V>>. We lose the badge of having no unsafe code, but the overall implementation is much simpler, relying on the battle-hardened hashbrown for the tricky parts. I have also confirmed that the testsuite passes under cargo miri.

I'll post the benchmark comparison in a followup comment, but broadly speaking it appears faster for insertion and lookup, and slower for removal. I think that's a reasonable trade-off for this crate.

As a bonus, I also implemented a proper reserve and added shrink_to_fit, since they are now pretty straightforward.

cc @Amanieu -- thanks for hashbrown and for exposing RawTable to make this possible!

[cuviper]

Note that this does raise the MSRV to 1.32, but that's well more than a year old. Perhaps we should release 1.4.1 for other recent changes, then bump to 1.5.0 for this change. We can also upgrade to 2018 edition, but I didn't want to pollute this PR with that churn.

cargo benchcmp master hashbrown

 name                                         master ns/iter  hashbrown ns/iter  diff ns/iter   diff %  speedup
 entry_hashmap_150                            2,533           2,476                       -57   -2.25%   x 1.02
 entry_indexmap_150                           3,809           2,976                      -833  -21.87%   x 1.28
 few_retain_hashmap_100_000                   1,016,146       1,005,607               -10,539   -1.04%   x 1.01
 few_retain_indexmap_100_000                  999,829         490,861                -508,968  -50.91%   x 2.04
 grow_fnv_hashmap_100_000                     1,995,356       1,757,197              -238,159  -11.94%   x 1.14
 grow_fnv_indexmap_100_000                    2,959,801       2,424,241              -535,560  -18.09%   x 1.22
 half_retain_hashmap_100_000                  973,803         1,005,521                31,718    3.26%   x 0.97
 half_retain_indexmap_100_000                 1,131,965       900,842                -231,123  -20.42%   x 1.26
 hashmap_merge_shuffle                        494,258         467,592                 -26,666   -5.40%   x 1.06
 hashmap_merge_simple                         370,419         384,379                  13,960    3.77%   x 0.96
 indexmap_clone_for_sort_s                    370,587         377,810                   7,223    1.95%   x 0.98
 indexmap_clone_for_sort_u32                  4,987           19,638                   14,651  293.78%   x 0.25
 indexmap_merge_shuffle                       432,270         356,746                 -75,524  -17.47%   x 1.21
 indexmap_merge_simple                        361,191         269,597                 -91,594  -25.36%   x 1.34
 indexmap_simple_sort_s                       1,890,796       1,953,812                63,016    3.33%   x 0.97
 indexmap_simple_sort_u32                     646,615         631,954                 -14,661   -2.27%   x 1.02
 indexmap_sort_s                              1,597,762       1,591,000                -6,762   -0.42%   x 1.00
 indexmap_sort_u32                            520,058         526,665                   6,607    1.27%   x 0.99
 insert_hashmap_100_000                       2,179,030       2,364,903               185,873    8.53%   x 0.92
 insert_hashmap_10_000                        195,234         209,327                  14,093    7.22%   x 0.93
 insert_hashmap_150                           2,923           3,116                       193    6.60%   x 0.94
 insert_hashmap_int_bigvalue_10_000           229,529         232,528                   2,999    1.31%   x 0.99
 insert_hashmap_str_10_000                    194,549         196,252                   1,703    0.88%   x 0.99
 insert_hashmap_string_10_000                 970,037         944,748                 -25,289   -2.61%   x 1.03
 insert_hashmap_string_10_000                 878,069         923,558                  45,489    5.18%   x 0.95
 insert_hashmap_string_oneshot_10_000         929,238         941,393                  12,155    1.31%   x 0.99
 insert_indexmap_100_000                      2,125,932       2,246,446               120,514    5.67%   x 0.95
 insert_indexmap_10_000                       213,993         189,754                 -24,239  -11.33%   x 1.13
 insert_indexmap_150                          3,210           2,926                      -284   -8.85%   x 1.10
 insert_indexmap_int_bigvalue_10_000          282,849         262,640                 -20,209   -7.14%   x 1.08
 insert_indexmap_str_10_000                   226,982         207,124                 -19,858   -8.75%   x 1.10
 insert_indexmap_string_10_000                881,052         879,212                  -1,840   -0.21%   x 1.00
 insert_indexmap_string_10_000                936,610         882,226                 -54,384   -5.81%   x 1.06
 iter_black_box_hashmap_10_000                18,282          18,481                      199    1.09%   x 0.99
 iter_black_box_indexmap_10_000               3,028           3,007                       -21   -0.69%   x 1.01
 iter_sum_hashmap_10_000                      10,546          10,419                     -127   -1.20%   x 1.01
 iter_sum_indexmap_10_000                     2,045           2,016                       -29   -1.42%   x 1.01
 lookup_hashmap_100_000_inorder_multi         68,768          69,260                      492    0.72%   x 0.99
 lookup_hashmap_100_000_multi                 72,178          70,700                   -1,478   -2.05%   x 1.02
 lookup_hashmap_100_000_single                15              16                            1    6.67%   x 0.94
 lookup_hashmap_10_000_exist                  68,488          68,528                       40    0.06%   x 1.00
 lookup_hashmap_10_000_exist_string           122,444         123,332                     888    0.73%   x 0.99
 lookup_hashmap_10_000_exist_string_oneshot   101,518         101,856                     338    0.33%   x 1.00
 lookup_hashmap_10_000_noexist                66,051          66,800                      749    1.13%   x 0.99
 lookup_indexmap_100_000_inorder_multi        82,405          87,341                    4,936    5.99%   x 0.94
 lookup_indexmap_100_000_multi                124,984         121,523                  -3,461   -2.77%   x 1.03
 lookup_indexmap_100_000_single               25              24                           -1   -4.00%   x 1.04
 lookup_indexmap_10_000_exist                 119,710         89,257                  -30,453  -25.44%   x 1.34
 lookup_indexmap_10_000_exist_string          176,353         165,409                 -10,944   -6.21%   x 1.07
 lookup_indexmap_10_000_exist_string_oneshot  160,672         131,624                 -29,048  -18.08%   x 1.22
 lookup_indexmap_10_000_noexist               120,897         74,732                  -46,165  -38.19%   x 1.62
 many_retain_hashmap_100_000                  374,578         381,946                   7,368    1.97%   x 0.98
 many_retain_indexmap_100_000                 891,856         1,165,316               273,460   30.66%   x 0.77
 new_hashmap                                  5               5                             0    0.00%   x 1.00
 new_indexmap                                 4               6                             2   50.00%   x 0.67
 pop_indexmap_100_000                         1,079,201       1,611,619               532,418   49.33%   x 0.67
 shift_remove_indexmap_100_000_few            14,464,051      15,427,596              963,545    6.66%   x 0.94
 shift_remove_indexmap_2_000_full             2,188,620       2,642,177               453,557   20.72%   x 0.83
 swap_remove_indexmap_100_000                 3,587,036       4,523,900               936,864   26.12%   x 0.79
 with_capacity_10e5_hashmap                   562             556                          -6   -1.07%   x 1.01
 with_capacity_10e5_indexmap                  1,511           215                      -1,296  -85.77%   x 7.03

[cuviper]

For future work, I've also been playing with parameterizing the index type, since rustc likes to use newtype-u32 indexes. We can extend to something like this without a breaking change:

pub struct IndexMap<K, V, S = RandomState, Idx = usize> { ... }
pub struct IndexSet<T, S = RandomState, Idx = usize> { ... }

Then internally we'd switch to RawTable<Idx>.

[bluss]

Really cool work.

So we lose the 32-bit index optimization, and still have some performance improvements, does that mean that there is more to gain?

We are giving up being implemented in safe Rust and can only do so if we show performance improvements that are above the noise. Some of the lookup benchmark cases do that, and the insert cases barely do it. The improved lookup benches are really encouraging.

I'll read RawTable and then come back to reviewing. Are you going to start experimenting with this version of indexmap in rustc, before it gets merged?

[cuviper]

So we lose the 32-bit index optimization, and still have some performance improvements, does that mean that there is more to gain?

Maybe, but that answer depends on whether I understand that optimization. 🙂

AIUI, the benefit of the 32-bit index is that we can stuff a short hash in the other 32-bits, which means we can use the hash without another memory access to entries. That's used for:

• Computing probe distance for Robin Hood hashing optimizations.
  • Hashbrown doesn't do Robin Hood, so this doesn't apply.
• Comparing 32-bit hashes as a short-circuit before checking key equality.
  • Hashbrown already keeps 7 bits of the hash in its control word, which it matches by SIMD before key equality. Of course that's not as good as 32-bit, but it's something.
• Using the short hash for reinsertion after resizing
  • We could potentially do this with hashbrown, but only if we're sure of the size class during every resizing operation.
  • In insert, hashbrown can avoid resizing even with zero capacity if the new value replaces a tombstone. We can't predict that from outside, so in order to control the size class, we'd have to force an unconditional reserve(1) beforehand.

Generally, I prefer the simplicity without those Pos hacks, especially since the performance holds. Also, that makes it easier to add in custom Idx types like I mentioned for future work.

Are you going to start experimenting with this version of indexmap in rustc, before it gets merged?

I do have local builds where I'm using this. So far it's a very slight improvement, less than 1%, but that's a good thing! For example, they reported a 21% slowdown in unused-warnings-check when switching to indexmap here, which I did confirm, but I see slightly improved performance with my new indexmap. That puts us in the realm of zero-cost abstractions, where FxIndexSet<T> performs similarly to a manual FxHashMap<T, Idx>, Vec<T> combination.

Here are those branches:
https://github.com/cuviper/indexmap/tree/shredded-potatoes-index-rustc
https://github.com/cuviper/rust/tree/indexmap

(You may need to adjust the rust Cargo.toml patch if you want to compile this yourself.)

[cuviper]

@bluss where do you stand on this? Just waiting for time to review it?

[bluss]

This weekend should have some time where I can review

[bluss]

There isn't much unsafe code, that's nice, but the questions of simultaneous reference and raw pointer validity stump me at the moment, so it's not trivial code.

[bluss]

Thanks all for your input on the discussion, and helping me find some solid footing again with the raw pointer code. Thanks cuviper for working on this - I'll work on helping you wrap this up as soon as you want.

There are two discomforting things left; I think it's something that's rather general to come up in this situation

• We handle raw buckets (raw pointer wrappers) that are passed in structs and between function scopes

  • Usually in Rust, we use safe abstractions, maybe just a simple lifetime to help us with the book keeping when leaving local reasoning. Lifetimes often help us convert the problem into just using local reasoning instead of global reasoning.
    I'm not saying it's worth the trouble to wrap this in further abstractions here, to make it safer, but it's the things we try to do if it's possible.

• We have internal functions that don't follow the usual safe/unsafe distinction

  • I believe it is best practice, that for example fn swap_remove_bucket(&mut self, raw_bucket: RawBucket) -> (usize, K, V),
    should be an unsafe method. The reason is that we must trust the caller to pass a correct raw bucket.
    I'll push a commit with this change.

  • We end up in a gray area here, since struct fields can not be made unsafe to assign (known issue), so we don't follow this completely stringently.

[bluss]

This might be the first time we raise the MSRV in 1.x, but with a good reason.

It looks like we might break serde_json, which builds CI with Rust 1.31? cc @dtolnay, when we raise ours to Rust 1.32 here.

Other top rdeps - toml, http, petgraph, they look fine from MSRV standpoint.

[cuviper]

• We handle raw buckets (raw pointer wrappers) that are passed in structs and between function scopes

  • Usually in Rust, we use safe abstractions, maybe just a simple lifetime to help us with the book keeping when leaving local reasoning. Lifetimes often help us convert the problem into just using local reasoning instead of global reasoning.
    I'm not saying it's worth the trouble to wrap this in further abstractions here, to make it safer, but it's the things we try to do if it's possible.

I'll play a little with abstracting this. It would be nice if we could contain unsafe even more tightly.

• We have internal functions that don't follow the usual safe/unsafe distinction

  • I believe it is best practice, that for example fn swap_remove_bucket(&mut self, raw_bucket: RawBucket) -> (usize, K, V),
    should be an unsafe method. The reason is that we must trust the caller to pass a correct raw bucket.
    I'll push a commit with this change.

  • We end up in a gray area here, since struct fields can not be made unsafe to assign (known issue), so we don't follow this completely stringently.

Yeah, this is kind of like the tootsie-pop model, just like Vec's length and capacity are innocent-looking fields with important safety implications.

This might be the first time we raise the MSRV in 1.x, but with a good reason.

If we entertain the idea of indexmap 2.0, are there other changes you would want?

It looks like we might break serde_json, which builds CI with Rust 1.31?

I am reluctant to push MSRV changes on others, but at least that is only for their non-default preserve_order feature. Still, they have CI to test that on 1.31, so they'd have to at least hold that back in CI to keep working. Such downgrades are possible, but annoying to deal with.

[cuviper]

I'll push a commit with this change.

That looks fine, thanks.

[bluss]

I don't think it's wrong to carefully raise the MSRV, it's the plan we have documented and promised since 1.0. However we can discuss it with those that depend on us.

[cuviper]

I tried an abstraction to wrap the raw bucket with an appropriate lifetime, by bundling to a map reference. We really only need that in OccupiedEntry, and I found that this structure basically is that wrapper already, so trying to add a separate layer just adds confusing indirection.

I settled instead on just encapsulating this in its own module. I moved map_core to map::core, then added a further map::core::raw for anything that has to deal with raw buckets, which is the only unsafe code.

[bluss]

I agree

[bluss]

I can't really think of any breaking changes that are on the wishlist.

I definitely don't want to pile on work here, but a 2.0 version could include the parameterization by index type. Then we can update the Default impl to be fully generic without having to fear type inference regressions (and the other new* methods too?).

The "experimental" things - Equivalent trait works well, the MutableKeys trait I don't know, so those things seem like they could stay as they are for 2.0.

Which method to make the default .remove() - I still think the current decision is fine, no change from my end for 2.0.
(Good way to document this would be to say that "remove is the fastest way to remove an element, and if order should be preserved, ...")

[cuviper]

I opened #135 for 2.0 discussion.

[bluss]

I'd vote to merge this and go for indexmap 1.5 with this

[jonhoo]

Not sure if my vote counts, but I agree :p

[cuviper]

I'm fine with that.

For new APIs in the release notes, we have reverse from #128, and this PR's shrink_to_fit and improved reserve. Maybe clone_from in #125 is worth mentioning, just to know that you can now clone into an existing allocation.

[cuviper]

I've added the version bump and some release notes to this PR.

[Amanieu]

insert_no_grow is now available in hashbrown 0.8.1.

[cuviper]

Great! I've updated to use the new hashbrown methods.

[cuviper]

Let's ship it!