Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

insert_unique_unchecked operation #293

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

stepancheg
Copy link
Contributor

Sometimes a map is constructed when it is known that all keys are
unique (e. e. if keys are coming from another map or from a
sorted/deduplicated iterator). In this case we can make insertion
faster by skipping a check that a key already exists in the map.

insert_unique_unchecked is guaranteed to be memory-safe, but does
not guarantee anything beyond that: if inserted key is not unique,
HashMap can panic, loop forever, return incorrect entry etc.

Added simple benchmark. insert_unique_unchecked is about 30%
faster than insert. Your mileage may vary of course.

Similar PR was
added to indexmap crate
and they asked to discuss the name of the operation with hashbrown
crate owners to come to the same naming convention (if hashbrown
is willing to have the same operation).

Sometimes a map is constructed when it is known that all keys are
unique (e. e. if keys are coming from another map or from a
sorted/deduplicated iterator). In this case we can make insertion
faster by skipping a check that a key already exists in the map.

`insert_unique_unchecked` is guaranteed to be memory-safe, but does
not guarantee anything beyond that: if inserted key is not unique,
`HashMap` can panic, loop forever, return incorrect entry etc.

Added simple benchmark. `insert_unique_unchecked` is about 30%
faster than `insert`.  Your mileage may vary of course.

Similar PR was
[added to `indexmap` crate](indexmap-rs/indexmap#200)
and they asked to discuss the name of the operation with `hashbrown`
crate owners to come to the same naming convention (if `hashbrown`
is willing to have the same operation).
@Amanieu
Copy link
Member

Amanieu commented Sep 12, 2021

Could you change this to return an OccupiedEntry or (&K, &mut V)? It doesn't cost anything and makes the API more useful.

@stepancheg
Copy link
Contributor Author

Regular insert doesn't do that (doesn't return a reference to the new entry).

What would be the use case for that reference? If a user need to modify an entry, they can do that before the insertion.

I can do it of course, just want to understand it.

@stepancheg
Copy link
Contributor Author

... we can't return OccupiedEntry because it contains a key which was used for lookup, which is not the case for insertion. But we can return (&K, &mut V).

@stepancheg
Copy link
Contributor Author

Added a commit on top which changes to return (&K, &mut V).

@stepancheg stepancheg force-pushed the insert-unique-unchecked branch from b3bdc62 to 7002f9f Compare September 12, 2021 22:59
@Amanieu
Copy link
Member

Amanieu commented Sep 13, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2021

📌 Commit 7002f9f has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Sep 13, 2021

⌛ Testing commit 7002f9f with merge 4fa0e2c...

@bors
Copy link
Contributor

bors commented Sep 13, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 4fa0e2c to master...

@bors bors merged commit 4fa0e2c into rust-lang:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants