Adaptive hashmap implementation

[arthurprs]

All credits to @pczarn who wrote rust-lang/rfcs#1796 and contain-rs/hashmap2#5

Background

Rust std lib hashmap puts a strong emphasis on security, we did some improvements in #37470 but in some very specific cases and for non-default hashers it's still vulnerable (see #36481).

This is a simplified version of rust-lang/rfcs#1796 proposal sans switching hashers on the fly and other things that require an RFC process and further decisions. I think this part has great potential by itself.

Proposal
This PR adds code checking for extra long probe and shifts lengths (see code comments and rust-lang/rfcs#1796 for details), when those are encountered the hashmap will grow (even if the capacity limit is not reached yet) greatly attenuating the degenerate performance case.

We need a lower bound on the minimum occupancy that may trigger the early resize, otherwise in extreme cases it's possible to turn the CPU attack into a memory attack. The PR code puts that lower bound at half of the max occupancy (defined by ResizePolicy). This reduces the protection (it could potentially be exploited between 0-50% occupancy) but makes it completely safe.

Drawbacks

• May interact badly with poor hashers. Maps using those may not use the desired capacity.
• It adds 2-3 branches to the common insert path, luckily those are highly predictable and there's room to shave some in future patches.
• May complicate exposure of ResizePolicy in the future as the constants are a function of the fill factor.

Example

Example code that exploit the exposure of iteration order and weak hasher.

const MERGE: usize = 10_000usize;
#[bench]
fn merge_dos(b: &mut Bencher) {
    let first_map: $hashmap<usize, usize, FnvBuilder> = (0..MERGE).map(|i| (i, i)).collect();
    let second_map: $hashmap<usize, usize, FnvBuilder> = (MERGE..MERGE * 2).map(|i| (i, i)).collect();
    b.iter(|| {
        let mut merged = first_map.clone();
        for (&k, &v) in &second_map {
            merged.insert(k, v);
        }
        ::test::black_box(merged);
    });
}

_91 is stdlib and _ad is patched (the end capacity in both cases is the same)

running 2 tests
test _91::merge_dos              ... bench:  47,311,843 ns/iter (+/- 2,040,302)
test _ad::merge_dos              ... bench:     599,099 ns/iter (+/- 83,270)

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[arthurprs]

The code is in very rough shape, I wanted to collect feedback on the idea first.

[alexcrichton]

r? @bluss

cc @pczarn, @apasel422

[bluss]

why was the capacity conditional removed here?

[arthurprs]

This doesn't need to be part of the PR. The capacity check is redundant though, right?

[bluss]

Right, it's the existence of it to start with that is puzzling, if capacity is 0, size is surely already 0.

[arthurprs]

I can remove that change. But the capacity check is redundant, right?

[arthurprs]

These of course will be moved into well commented constants.

[arthurprs]

I wonder if we can get away only checking probe length. It's still possible to abuse long shifts without hitting the probe length limit but that's a lot harder.

[bluss]

Looks remarkably simple for what it does. That's good.

Advantages:

• Only affects insert (the fast lookup we have is untouched)
• Simple

Obviously the constants involved need constant names, tuning and comments. I think we can make an argument that for example a displacement of 128 slots from its best position is always a bad case and should never occur in a healthy hash table, no matter its size?

[arthurprs]

The good thing is that the math behind this is independent from the map size, it's only a function of the fill factor and the hasher being good. The first is fine as the constants work for fill factors smaller than the one they were calculated.

Interacting badly with bad hashers could be problematic in practice as hashmap may never reach the maximum fill factor (the check for half filled is useful here so it doesn't blow up).

[Veedrac]

We need a lower bound on the minimum occupancy that may trigger the early resize, otherwise in extreme cases it's possible to turn the CPU attack into a memory attack. The PR code puts that lower bound at half of the max occupancy (ResizePolicy).

This sounds like a good idea, but it means it only counters the n=2 case (aka. merging two maps, rather than, say, the first nth of n maps). That's definitely an improvement, though.

[arthurprs]

@Veedrac what do you mean by n=2 case?

[arthurprs]

Putting into more generic terms, you mean that it can still be abused while between 0% and 50% filled?

[Veedrac]

Yes, basically. I'll try to cook up some examples later, to give a more concrete demo.

[arthurprs]

Trying to resume the conversation... I think the obvious open question here is the interaction with less than good hashers, hashmaps using those may not use the desired capacity.

[alexcrichton]

The libs team discussed this briefly at triage the other day, and we were wondering if we could perhaps land this ahead of the RFC? The changes to probing here are universally better, even if we don't do the hasher changes yet, right?

If so perhaps, the PR title/description could be cleaned up to the current state and we could look to merge?

[arthurprs]

I'll update the PR/description to provide clearer picture.

The changes to probing here are universally better, even if we don't do the hasher changes yet, right?

I wouldn't say universally, but mostly.

[alexcrichton]

Ah ok, thanks for the clarification. Want to ping me when updated and we can look to merge?

[arthurprs]

I should have elaborated that. It's not strictly better because the interaction with poor hashers isn't great, with those it's possible that the hashmap resizes early even on non-rogue input.

I'll ping when I update it.

[arthurprs]

PR updated, now there's two constants and lots of comment lines.

[alexcrichton]

Thanks @arthurprs! Out of curiosity, would it be at all possible to add a test for this?

[arthurprs]

I think so. It's possible to observe the early resizes from the public api and it's somewhat easy to trigger it by merging two maps with the same hash seed (like the example in first post). I'll write something tomorrow.

[arthurprs]

I rebased and squashed the commits.

[alexcrichton]

@bors: r+

Thanks again and for being patient @arthurprs!

[bors]

📌 Commit 57940d0 has been approved by alexcrichton

[bors]

⌛ Testing commit 57940d0 with merge 668864d...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 668864d to master...

[arthurprs]

@istankovic Please make a PR 😃

[istankovic]

@arthurprs Nah, it was just something I noticed so I made the comments, but it doesn't bother me enough to make a PR, sorry...

[arthurprs]

The shift length math is broken. It turns out that checking for the shift length is complicated. Using simulations it's possible to see that a value of 2000 will only get probabilities down to ~1e-7 when the hashmap load factor is 90% (rust goes up to 90.9% as of today). That's probably not good enough to go into the stdlib with pluggable hashers. See rust-lang/rfcs#1796 (comment) and rust-lang/rfcs#1796 (comment)

I suggest taking that part out and keeping only displacement check, which is much safer and very useful by itself.

cc @pczarn @bluss @alexcrichton

[pczarn]

I agree. This issue also indicates that the hashmap load factor may be too high.

Thanks for help running these simulations.

[SimonSapin]

Because of alignment(?), this one bool field makes the already-large HashMap grow from 40 bytes to 48. Maybe it’s worth the space, but I thought this should be noted.

(We caught this in Servo because we have unit tests that check std::mem::size_of for various types used in DOM nodes, to catch size regressions. Because there can be so many nodes in a document, even a small size increase can be significant.)

[SimonSapin]

Could the extra bit be packed in RawTable::size or RawTable::capacity?

[pczarn]

Yes, of course. The code is going to be messy, though.

If we're able to restrict adaptive hashing to maps with the default hasher, I'd prefer to have the extra bit in RandomSipHasher.

[arthurprs]

It's just a matter of finding how to use the bit with reasonable code.

I'd argue against making it RandomState only, the selling point was supporting all hashmaps. Edit: I also think that making it RandomState only will require even more code.

[SimonSapin]

Let’s discuss in #40042.