Changed HashMap's internal layout. Cleanup. #21973

pczarn · Feb 5, 2015

Changes HashMap's memory layout from [hhhh...KKKK...VVVV...] to [KVKVKVKV...hhhh...]. This makes in-place growth easy to implement (and efficient).

The removal of find_with_or_insert_with has made more cleanup possible.

20 benchmark runs, averaged:

                           before         after
bench::find_existing       40573.50 ns    41227.65 ns
bench::find_nonexisting    41815.45 ns    42362.60 ns
bench::get_remove_insert     197.85 ns      198.60 ns
bench::grow_by_insertion     171.05 ns      154.05 ns
bench::hashmap_as_queue      112.85 ns      112.65 ns
bench::new_drop               79.40 ns       79.20 ns
bench::new_insert_drop       179.40 ns      149.05 ns

thanks to @Gankro for the Entry interface, and to @thestinger for improving jemalloc's in-place realloc!
cc @cgaebel
r? @Gankro

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs

+// middle of a cache line, this strategy pulls in one cache line of hashes on
+// most lookups (64-byte cache line with 8-byte hash). I think this choice is
+// pretty good, but α could go up to 0.95, or down to 0.84 to trade off some
+// space.
 //
 // > Wait, what? Where did you get 1-α^k from?


This should be updated, too.

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs

@@ -126,23 +150,10 @@ fn test_resize_policy() {
 // α^3, etc. Therefore, the odds of colliding k times is α^k. The odds of NOT
 // colliding after k tries is 1-α^k.
 //
-// The paper from 1986 cited below mentions an implementation which keeps track


Why was this removed?

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs

@@ -220,11 +231,12 @@ fn test_resize_policy() {
 ///
 /// Relevant papers/articles:
 ///
-/// 1. Pedro Celis. ["Robin Hood Hashing"](https://cs.uwaterloo.ca/research/tr/1986/CS-86-14.pdf)


Why remove this citation?

To avoid confusion. It doesn't seem relevant to this particular implementation

I think it's relevant. It's the seminal paper on robin hood hashing, no?

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs

    let size = table.size();
-    let mut probe = Bucket::new(table, hash);
+    let mut probe = if let Some(probe) = Bucket::new(table, hash) {


s/if let/match

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs

-            if is_match(full.read().0) {
-                return FoundExisting(full);
+            if is_match(bucket.read().1) {
+                return InternalEntry::Occupied(OccupiedEntryState {


I don't know what the changes in this function have to do with in-place hashmap growth.

cgaebel · Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel · Feb 5, 2015

src/libstd/collections/hash/map.rs


    loop {
-        let (old_hash, old_key, old_val) = bucket.replace(hash, k, v);
+        let (old_hash, old_key, old_val) = {


What's wrong with what was there before?

I tried to get rid of non-essential methods in the table module. Besides, replace was used only from here.

cgaebel · Feb 5, 2015

Can this PR be split up into "In-place growth for HashMap" and "Cleanup"?

Gankro · Feb 5, 2015

Just leaving a note: I am concerned about how this change will negatively affect memory consumption for certain choices of K and V.

That said, I think speed is more important than memory consumption, to some limit.

cgaebel · Feb 5, 2015

It will also affect the performance of the "keys" and "values" iterators.

pnkfelix · Feb 12, 2015

Don't the posted benchmark results indicate that insertion (grow_by_insertion and new_insert_drop) have become faster at the expense of making lookup (find_existing, find_nonexisting) slower?

(This outcome makes some sense to me, at least for a heavily loaded table, since the unused interleaved values for non-matching keys are going to occupy portions of the cache line when we are doing a probe sequence over a series of keys.)

(( Well, maybe this explanation is a little too simplistic; the sequence of hhhh ... at the end i guess indicates that in the common case, we should need to only look at a series of hashcodes before we start inspecting the keys themselves, so its not quite as dire as the above explanation made it out to be. ))

I don't know how to evaluate whether the gain is worth the cost here. I just want to make sure that everyone is on board for this shift (or find out if my interpretation of the results is wrong).

Gankro · Feb 12, 2015

Just a note that I believe @pczarn is currently reworking this PR.

pczarn · Feb 16, 2015

I did the reworking. Some small details still need attention.

It will also affect the performance of the "keys" and "values" iterators.

Sounds bad. Can you find a real world example of this? To keep the performance the same, I could make hashmap use two allocations, [VVVV…hhhh…] and [KKKK…].

Here's a relevant post on data layout: http://www.reedbeta.com/blog/2015/01/12/data-oriented-hash-table/
Unfortunately, it's of little use for us, because it's not about Robin Hood (and for C++, not Rust).

This is how benchmark results have changed:

[40.573, 41.706, 209.1, 179.4, 122.8, 78.4, 182.7]
  Cleanup.
[41.989, 43.615, 212.4, 167.8, 125.0, 78.7, 163.3]
  Changed HashMap's internal layout.
[41.145, 42.298, 206.8, 162.5, 121.7, 78.9, 159.3]
  In-place growth for HashMap.
[41.055, 42.427, 208.3, 156.0, 124.2, 78.4, 161.6]

@pnkfelix: Lookup has become slower after the first commit because of refactoring. Keep in mind that the benchmark does 1000 lookups per iteration and the difference between 41.7ns and 42.4ns per lookup is small.

The improvement from in-place growth is suprisingly low. I'll have to check why.

cgaebel · Feb 16, 2015

@pczarn The performance of key/value iterators is just because with the current design they walk over a compact array, and with your proposed design they eat twice as much cache as they do this. If doing a small per-key or per-value operation, this essentially halves memory bandwidth.

Gankro · Feb 16, 2015

Well strictly speaking it's already walking over the hashes checking for hash != 0 at the same time.

Gankro · Feb 21, 2015

Oh shoot, I let this slip through the cracks. Needs a rebase (hopefully the last one, since we should be done with crazy API churn).

pczarn · Feb 21, 2015

Updated. I'm going to test and make corrections when new snapshots land.

So iteration over small keys/values is already 2x-4x more cache-intensive than in an array. With larger values, like in HashMap<usize, (String, String)>, it gets much worse.

To avoid the issue, keys/values can be stored in an array such as [([K; 16], [V; 16]); n].

pczarn · Feb 27, 2015

Done, tested.

Gankro · Feb 27, 2015

Great! Will review tonight.

Gankro · Feb 27, 2015

Ack, a few of my issues are addressed by later commits. Doing this commit-by-commit isn't the right strategy here. Shifting gears.

Gankro · Feb 27, 2015

Gankro reviewed Feb 27, 2015

View changes

Gankro · Feb 27, 2015

src/libstd/collections/hash/table.rs

-    // inform rustc that in fact instances of K and V are reachable from here.
-    marker:   marker::PhantomData<(K,V)>,
+    // NB. The table will probably need manual impls of Send and Sync if this
+    // field ever changes.


I don't think that needs saying? That's true for pretty much all Uniques.

Gankro · Feb 27, 2015

Gankro reviewed Feb 27, 2015

View changes

Gankro · Feb 27, 2015

src/libstd/collections/hash/table.rs

@@ -69,50 +67,42 @@ const EMPTY_BUCKET: u64 = 0u64;
 pub struct RawTable<K, V> {
    capacity: usize,
    size:     usize,
-    hashes:   Unique<u64>,


interest piqued

bors · Apr 22, 2015

☔️ The latest upstream changes (presumably #24674) made this pull request unmergeable. Please resolve the merge conflicts.

Manishearth · May 10, 2015

@Gankro (this was rebased and needs review)

Gankro · May 10, 2015

@Manishearth Yes I tried to get someone else to review over two months ago. :/

Today's that last day of my pseudo-vacation so I'm free to tackle this again this week.

Gankro · May 12, 2015

Alright I've started just reading the full source at the last commit's hash just because the changes have been so comprehensive that it borders on a rewrite.

Was Deref vs Borrow ever fully addressed? Borrow is generally just a trait for Deref-like+Hash/Eq/Ord equivalence.

Gankro · May 12, 2015

Gankro reviewed May 12, 2015

View changes

Gankro · May 12, 2015

src/libstd/collections/hash/table.rs

-    pub fn into_bucket(self) -> Bucket<K, V, M> {
+    /// Duplicates the current position. This can be useful for operations
+    /// on two or more buckets.
+    pub fn stash(self) -> Bucket<K, V, Bucket<K, V, M, S>, S> {


What about -> Bucket<K, V, Self, S>?

Gankro · May 15, 2015

Gankro reviewed May 15, 2015

View changes

Gankro · May 15, 2015

src/libstd/collections/hash/table.rs

-        }
+    /// Pointer to one-past-the-last key-value pair.
+    pub fn as_mut_ptr(&self) -> *mut (K, V) {
+        unsafe { self.middle.get() as *const _ as *mut _ }


Is the as *const _ necessary?

Gankro · May 15, 2015

Gankro reviewed May 15, 2015

View changes

Gankro · May 15, 2015

src/libstd/collections/hash/table.rs

+fn checked_size_generic<K, V>(capacity: usize) -> usize {
+    let size = size_generic::<K, V>(capacity);
+    let elem_size = size_of::<(K, V)>() + size_of::<SafeHash>();
+    assert!(size >= capacity.checked_mul(elem_size).expect("capacity overflow"),


I'm confused why this assert instead of just constructing size using checked ops?

Gankro · May 15, 2015

Gankro reviewed May 15, 2015

View changes

Gankro · May 15, 2015

src/libstd/collections/hash/table.rs

+
+#[inline]
+fn align<K, V>() -> usize {
+    cmp::max(mem::min_align_of::<(K, V)>(), mem::min_align_of::<u64>())


u64 -> SafeHash?

Gankro · May 15, 2015

Gankro reviewed May 15, 2015

View changes

Gankro · May 15, 2015

src/libstd/collections/hash/table.rs

+}
+
+/// A newtyped RawBucket. Not copyable.
+pub struct RawFullBucket<K, V, M>(RawBucket<K, V>, PhantomData<M>);


So many new types @_@

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/bench.rs

+    let mut m = HashMap::new();
+
+    for i in range_inclusive(1, 1000) {
+        m.insert(i, (String::new(), String::new()));


PR #999999: Optimize HashMap by reducing size of String

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/map.rs

+impl<K, V, M> InternalEntry<K, V, M> {
+    fn into_option(self) -> Option<FullBucket<K, V, M>> {
+        match self {
+            InternalEntry::Occupied(bucket) => Some(bucket.elem),


Oh lord, a bucket's elem is a bucket?

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/map.rs

        }

        // If the hash doesn't match, it can't be this one..
-        if hash == full.hash() {
+        if hash == *bucket.read().0 {


full.hash() seemed like a lot clearer of an API...

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/map.rs

-            TableRef(_) => None
-        }
+// Performs insertion with relaxed requirements.
+// The caller should ensure that invariants of Robin Hood linear probing hold.


Is this the only requirement that's relaxed? Unclear.

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/map.rs

+    let mut buckets = Bucket::new(table, *hash as usize).unwrap();
+    let ib = buckets.index();
+
+    while buckets.index() != ib + cap {


This is equivalent to buckets.displacement() != cap, right?

Scratch that, obviously not available on a "pure" bucket.

Gankro · May 18, 2015

Gankro reviewed May 18, 2015

View changes

Gankro · May 18, 2015

src/libstd/collections/hash/map.rs

@@ -596,6 +590,8 @@ impl<K, V, S> HashMap<K, V, S>
    }

    /// Returns the number of elements the map can hold without reallocating.
+    /// This value may be lower than the real number of elements the map will
+    /// hold before reallocating.


Gankro · May 19, 2015

So, to the best of my knowledge this code seems to be correct, but I have strong reservations about where we're going with respect to the proliferation of type-complexity. I used to be able to grok this code pretty ok: We have HashMap, RawTable, and some Buckets. Now everything's super generic over some "M" type and there's Partial* types all over. It's not clear that these abstractions are pulling their weight: are they preventing real bugs or enabling simpler or more maintainable code?

CC @nikomatsakis @huonw @cgaebel

cgaebel · May 19, 2015

I share that sentiment.

Gankro · May 28, 2015

It's been a couple weeks with no response on any of my comments, and I'm not a huge fan of the general design changes. As such I'm closing this for now. We can continue discussion on the PR and maybe re-open if we get somewhere.

rust-highfive assigned Gankro Feb 5, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from dc6bc2d to 64e8b91 Feb 5, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 64e8b91 to 407572c Feb 5, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 49bfe41 to f495fb8 Feb 7, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from f495fb8 to 6218462 Feb 16, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 6218462 to 316b300 Feb 21, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 627837f to edcddb1 Feb 26, 2015

pczarn referenced this pull request Feb 26, 2015
Closed
llvm::StructType::getElementType: Assertion `N < NumContainedTys && "Element number out of range!"' failed. #21721

pczarn force-pushed the pczarn:hash_map-mem-layout branch from edcddb1 to b7317cd Feb 27, 2015

Cleanup. Changed HashMap's internal layout.
* use of NonZero hashes * refactoring * correct explanation of the load factor * better nomenclature * 'probe distance' -> 'displacement'

Loading status checks…

776d23e

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 3f67ec1 to 776d23e Apr 29, 2015

alexcrichton added the T-libs label May 26, 2015

Gankro closed this May 28, 2015

This was referenced Sep 20, 2016

Closed

Use `usize` instead of `u64` for hashes in HashMap #36567

Closed

Revisit HashMap memory layout #36660

Merged

Cache conscious hashmap table #36692

rust-lang/rust

Join GitHub today

Changed HashMap's internal layout. Cleanup. #21973

Conversation

pczarn commented Feb 5, 2015

rust-highfive assigned Gankro Feb 5, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from dc6bc2d to 64e8b91 Feb 5, 2015

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 64e8b91 to 407572c Feb 5, 2015

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

cgaebel reviewed Feb 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cgaebel commented Feb 5, 2015

This comment has been minimized.

Gankro commented Feb 5, 2015

This comment has been minimized.

cgaebel commented Feb 5, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 49bfe41 to f495fb8 Feb 7, 2015

This comment has been minimized.

pnkfelix commented Feb 12, 2015

This comment has been minimized.

Gankro commented Feb 12, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from f495fb8 to 6218462 Feb 16, 2015

This comment has been minimized.

pczarn commented Feb 16, 2015

This comment has been minimized.

cgaebel commented Feb 16, 2015

This comment has been minimized.

Gankro commented Feb 16, 2015

This comment has been minimized.

Gankro commented Feb 21, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 6218462 to 316b300 Feb 21, 2015

This comment has been minimized.

pczarn commented Feb 21, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 627837f to edcddb1 Feb 26, 2015

pczarn referenced this pull request Feb 26, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from edcddb1 to b7317cd Feb 27, 2015

This comment has been minimized.

pczarn commented Feb 27, 2015

This comment has been minimized.

Gankro commented Feb 27, 2015

This comment has been minimized.

Gankro commented Feb 27, 2015

Gankro reviewed Feb 27, 2015 View changes

This comment has been minimized.

Gankro reviewed Feb 27, 2015 View changes

This comment has been minimized.

This comment has been minimized.

bors commented Apr 22, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from 3f67ec1 to 776d23e Apr 29, 2015

This comment has been minimized.

Manishearth commented May 10, 2015

This comment has been minimized.

Gankro commented May 10, 2015

This comment has been minimized.

Gankro commented May 12, 2015

Gankro reviewed May 12, 2015 View changes

This comment has been minimized.

Gankro reviewed May 15, 2015 View changes

This comment has been minimized.

Gankro reviewed May 15, 2015 View changes

This comment has been minimized.

Gankro reviewed May 15, 2015 View changes

This comment has been minimized.

Gankro reviewed May 15, 2015 View changes

This comment has been minimized.

Gankro reviewed May 18, 2015 View changes

This comment has been minimized.

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `dc6bc2d` to `64e8b91` Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel reviewed Feb 5, 2015

View changes

cgaebel reviewed Feb 5, 2015

View changes

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `64e8b91` to `407572c` Feb 5, 2015

cgaebel reviewed Feb 5, 2015

View changes

cgaebel reviewed Feb 5, 2015

View changes

cgaebel reviewed Feb 5, 2015

View changes

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `49bfe41` to `f495fb8` Feb 7, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `f495fb8` to `6218462` Feb 16, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `6218462` to `316b300` Feb 21, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `627837f` to `edcddb1` Feb 26, 2015

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `edcddb1` to `b7317cd` Feb 27, 2015

Gankro reviewed Feb 27, 2015

View changes

Gankro reviewed Feb 27, 2015

View changes

pczarn force-pushed the pczarn:hash_map-mem-layout branch from `3f67ec1` to `776d23e` Apr 29, 2015

Gankro reviewed May 12, 2015

View changes

Gankro reviewed May 15, 2015

View changes

Gankro reviewed May 15, 2015

View changes

Gankro reviewed May 15, 2015

View changes

Gankro reviewed May 15, 2015

View changes

Gankro reviewed May 18, 2015

View changes

Gankro reviewed May 18, 2015

View changes

Gankro reviewed May 18, 2015

View changes

Gankro reviewed May 18, 2015

View changes

Gankro reviewed May 18, 2015

View changes

Gankro reviewed May 18, 2015

View changes