IndexMap::insert_unique_unchecked

[stepancheg]

Insert a key-value pair into the map without checking if the key
already exists in the map.

This operation is safe if a key does not exist in the map.

However, if a key exists in the map already, the behavior is
unspecified: this operation may panic, or any following operation
with the map may panic or return arbitrary result.

This operation is faster than regular insert, because it does not
perform lookup before insertion.

This operation is useful during initial population of the map. For
example, when constructing a map from another map, we know that
keys are unique.

Simple benchmark of insert vs insert_unique_unchecked included:

test insert                  ... bench:      14,929 ns/iter (+/- 2,222)
test insert_unique_unchecked ... bench:      11,272 ns/iter (+/- 1,172)

[bluss]

Does hashbrown itself have any operation like this? Maybe insert_unique_unchecked or insert_new_unchecked would be a name that the user understands more intuitively? An IndexSet method with the same semantics is needed to maintain parity, if this method is accepted.

If this method is good - is it something we can use internally somewhere?

[cuviper]

Hashbrown's several RawTable::*insert* methods works this way, where you're expect to check matches separately first. We do take advantage of that in the push being used here, e.g. via VacantEntry::insert, or in our rebuild_hash_table calling insert_no_grow.

I'm not aware of anything public like this on hashbrown::HashMap, but it would be nice to settle on a good name together...

[stepancheg]

I like insert_unique_unchecked name.

Give me some time, I'll open a PR to hashbrown and and hear what people say there (about name, and generally about the idea of exposing map internals). One possibility is that they say, just use RawTable directly. And maybe this is what indexmap crate should do to stay consistent with others.

[stepancheg]

Hashbrown PR: rust-lang/hashbrown#293

[stepancheg]

Updated the PR with insert_unique_unchecked name to match new hashbrown API.

However, unlike hashbrown, I opted not to return (&K, &mut V) from insert_unique_unchecked because returning these values caused significant regression even in insert operation (if IndexMapCore::push returns (usize, &K, &mut V) instead of usize). It should be no regression, but perhaps something is inefficient in generated machine code.

[bluss]

Looks good! It does not return a reference in this version (this doc and the other both say this).

I can see the problem, we'd need some unsafe code to do the index -> &mut V lookup without extra cost here. It's fine to avoid this and just return nothing I think (?) Unless there is a compelling nameable usecase

[bluss]

@cuviper This is a new feature and in theory we'd bump the version to 1.8 since it's a feature update. I guess it's only debatable where to draw the line between incremental fix and improvement, but I don't mind using the feature bump for new methods (for new trait impls, depends, then it depends I think).

[bluss]

I prefer to see that we try to keep map-set parity when possible. Should there be a set method for this?

[stepancheg]

Updated docs.

There's a IndexSet::insert_unique_unchecked operation in the latest version of the PR.

[cuviper]

Updated the PR with insert_unique_unchecked name to match new hashbrown API.

However, unlike hashbrown, I opted not to return (&K, &mut V) from insert_unique_unchecked because returning these values caused significant regression even in insert operation (if IndexMapCore::push returns (usize, &K, &mut V) instead of usize). It should be no regression, but perhaps something is inefficient in generated machine code.

I'm conflicted about matching the name without properly matching the API in the return value. I don't see why you would change push though -- e.g. VacantEntry::insert just calls push and then does the indexing itself.

[cuviper]

Simple benchmark of insert vs insert_unique_unchecked included:

test insert                  ... bench:      14,929 ns/iter (+/- 2,222)
test insert_unique_unchecked ... bench:      11,272 ns/iter (+/- 1,172)

FWIW, the initial numbers are much closer on my machine, using rustc 1.57.0-nightly (5ecc8ad84 2021-09-19):

test insert                  ... bench:      12,863 ns/iter (+/- 171)
test insert_unique_unchecked ... bench:      11,312 ns/iter (+/- 114)

Adding the return references does consistently slow that down:

test insert                  ... bench:      12,875 ns/iter (+/- 298)
test insert_unique_unchecked ... bench:      11,917 ns/iter (+/- 96)

---

I also tried running with 10k insertions, and the benefit narrowed further:

test insert_10k                  ... bench:     158,763 ns/iter (+/- 1,472)
test insert_unique_unchecked_10k ... bench:     143,972 ns/iter (+/- 1,155)

and with return references:

test insert_10k                  ... bench:     158,910 ns/iter (+/- 2,191)
test insert_unique_unchecked_10k ... bench:     149,291 ns/iter (+/- 1,396)

[stepancheg]

@cuviper looks bad. Let me check again.

[stepancheg]

On my laptop, MacBook Pro, Intel Core i9, rustc 1.57.0-nightly (8c2b6ea37 2021-09-11) the numbers I gave are correct.

[stepancheg]

For 10k items:

test insert                  ... bench:     137,650 ns/iter (+/- 13,904)
test insert_unique_unchecked ... bench:     116,824 ns/iter (+/- 25,202)

For 100:

test insert                  ... bench:       1,530 ns/iter (+/- 141)
test insert_unique_unchecked ... bench:       1,288 ns/iter (+/- 237)

[cuviper]

@bluss

This is a new feature and in theory we'd bump the version to 1.8 since it's a feature update. I guess it's only debatable where to draw the line between incremental fix and improvement, but I don't mind using the feature bump for new methods (for new trait impls, depends, then it depends I think).

I'm not too concerned about that version criteria, personally. I do prefer minor bumps for pseudo-breaking things like MSRV, but otherwise it's fine either way. We also have #195 and #196 pending release that are featureful.

@stepancheg

On my laptop, MacBook Pro, Intel Core i9, rustc 1.57.0-nightly (8c2b6ea37 2021-09-11) the numbers I gave are correct.

That's fine, benchmarks vary. FWIW mine is a desktop running Fedora 34 with an AMD Ryzen 7 3800X.
(I'm surprised your 10k numbers are so much better when the others are pretty close!)

[stepancheg]

I don't see why you would change push though -- e.g. VacantEntry::insert just calls push and then does the indexing itself.

And if I do that, there's no regression anymore (at least I don't see it easily: rust builtin benchmarks are too unreliable). I'll update the PR now.

[stepancheg]

OK, proper (more or less) benchmark for 1000 inserts. 100 runs.

Raw data: https://gist.github.com/stepancheg/b234abc8da06de88acc18c3a1c7adfe3
Averages of 100 run averages:
insert: 15.632
insert_unique_unchecked: 13.105

Yes, I gave a too high number initially. Sorry.

[stepancheg]

I'll post an update after another benchmark finishes.

[stepancheg]

OK, more benchmarks. Returning references from insert_unique_unchecked as implemented in current version of PR (969115b) actually causes small regression for insert_unique_unchecked operation compared to returning nothing, but regression is even smaller when using unsafe.

How I checked.

I created a simple utility:

#![feature(bench_black_box)]

use indexmap::IndexMap;
use std::hint::black_box;

fn main() {
    for _ in 0..200000 {
        let mut m = IndexMap::with_capacity(1000);
        for i in 0..1000 {
            m.insert_unique_unchecked(i, i);
            black_box(&mut m);
        }
    }
}

Compiled it with this branch.

Then applied a patch which removes returning values and compiled the utility again (version C).

And applied a patch to this branch which uses unsafe (version B):

--- a/src/map.rs
+++ b/src/map.rs
@@ -390,10 +390,11 @@ where
     /// This operation is useful during initial population of the map.
     /// For example, when constructing a map from another map, we know
     /// that keys are unique.
+    #[allow(unsafe_code)]
     pub fn insert_unique_unchecked(&mut self, key: K, value: V) -> (&K, &mut V) {
         let hash = self.hash(&key);
         let index = self.core.push(hash, key, value);
-        let entry = &mut self.core.as_entries_mut()[index];
+        let entry = unsafe { self.core.as_entries_mut().get_unchecked_mut(index) };
         (&entry.key, &mut entry.value)
     }

Then I used my absh utility https://github.com/stepancheg/absh which I used for proper A/B benchmarking.

Utility is invoked like this:

absh -a ./default -b ./with-unsafe -c ./no-return -r

After 500 iterations the output is:

A: n=501 mean=2.556 std=0.129 se=0.005 min=2.355 max=3.073 med=2.526
B: n=501 mean=2.497 std=0.128 se=0.005 min=2.313 max=3.203 med=2.462
C: n=501 mean=2.479 std=0.139 se=0.006 min=2.273 max=2.975 med=2.438
A: distr=[      ▁▃▅▆▇▅█▃▆▆▅▂▄▄▃▂▂▃▃▂▁▁▁▁ ▂  ▁                      ]
B: distr=[   ▁▃▆▆▆██▆▇▅▃▄▄▄▁▃▂▁▃▂▁ ▁▁▁▁▁  ▁                        ]
C: distr=[ ▁▄▄▄▆▆█▇▅▅▅▅▃▃▃▃▁▃▁▂▁▁▁▁▁▁▁▁▁▁▁   ▁                     ]
B/A: 0.977 0.971..0.983 (95% conf)
C/A: 0.970 0.963..0.976 (95% conf)

Again:
A: this branch with returning references
B: this branch with returning references with unsafe
C: this branch without returning references

So, simply returning references (A) causes about 3% regression against version with returning nothing (C), but using unsafe wins most of it back.

[Sagebati]

Hi, this is an interesting feature, what can I do too push it forward ? The PR in hashbrown got merged. Do we need it also on the HashMap of std ?

[cuviper]

The ideal would be to have this added and stabilized in std first, so the API precedent is locked in. AFAIK, that hasn't even been proposed yet, which should go through this process:
https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html