Unify the entry and insert code

[mwillsey]

This will close #108

This doesn't so much improve the entry code rather than abstract away entry and insert into a ProbeAction. This forces monomorphization to give us the insert fast path that doesn't require the additional indirection that entry does.

It also implements insert_full on that fast path (insert is now just a call to insert_full), so that should be much faster as well, since it was formerly implemented in terms of entry.

Benchmarks

 name                                   control ns/iter  variable ns/iter  diff ns/iter  diff %  speedup 
 insert_orderedmap_100_000              2,688,143        2,616,739              -71,404  -2.66%   x 1.03 
 insert_orderedmap_10_000               275,198          272,283                 -2,915  -1.06%   x 1.01 
 insert_orderedmap_150                  4,116            3,862                     -254  -6.17%   x 1.07 
 insert_orderedmap_int_bigvalue_10_000  409,681          395,290                -14,391  -3.51%   x 1.04 
 insert_orderedmap_str_10_000           298,722          294,373                 -4,349  -1.46%   x 1.01 
 insert_orderedmap_string_10_000        2,027,784        1,948,038              -79,746  -3.93%   x 1.04 

[bluss]

this could be just entry.insert_impl::<Sz>(self.0) right? Something like that to reduce duplication - even hit could use that(?)

[mwillsey]

Ah good idea. Do you mean empty could use that? I was afraid that entering the probe loop another time would be costly (technically, you don't have to in the empty case), but it seems to be fine.

[mwillsey]

Nvm, turns out doing it in empty isn't great, it basically removes the fast path that the old insert code had, making steal and empty the same.

[bluss]

Are we then maybe identifying the main thing that slows down entry compared with the old insert? Maybe if we updated entry's VacantEntry to skip that loop, for the non-steal case, that insert and entry would match each other in performance?

[bluss]

That's awesome

[bluss]

Could you update the PR description to use one of the formulations that means that #108 is closed when this is merged? :) https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[mwillsey]

Done!

[cuviper]

LGTM -- just one typo noted...

[bluss]

Thanks. I'll merge this when I have time to make some post-merge edits

[cuviper]

@bluss can I help with whatever is blocking this?

[bluss]

I'll add it as comments. I thought I might as well merge and edit myself, sometimes I prefer that to the back-and-forth, just don't want reviews to be too perfectionist

[bluss]

Thanks a lot for making this PR and improving indexmap. I just put comments on the small things I'd edit if I had time

[bluss]

This method body seems "loose" like this - could it not just call a method on the entry? I general, it's hard to follow the insert logic when it's spread out.

(This comment is rather perfectionist, and not something I wanted to make a requirement to fix before merging).

[cuviper]

I looked at this -- you'd basically want VacantEntry::insert_impl without the probe loop (since we know its empty). But this needs to consume the entry's key, which makes it harder to reuse that code, so I think we'd end up writing this code separately either way. i.e. It doesn't really matter whether we do it here or there.

(And frankly, a lot of this will disappear if I land the hashbrown changes...)

[cuviper]

Yeah, I get that, and sometimes I'll take advantage of maintainer pushes to PRs for that same reason.

I'm pressing because I'm building up changes toward using indexmap more in rustc. 🙂

[cuviper]

I fixed the spacing nit, and I think we can wait on the other point.

Thanks @mwillsey!