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

Add reinsert to RawOccupiedEntryMut #450

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

1kjo
Copy link

@1kjo 1kjo commented Aug 3, 2023

It is sometimes useful to modify the key associated with an entry without changing the value. For example, when building a cache where keys can change.

It is currently possible to implement this using hashbrown::HashMap by removing the entry and adding a new one with the same value. However, to prevent conflicts with the new key, the code has to:

  1. Lookup the entry with the new key, and return an error if it occupied.
  2. Delete the entry with the old key.
  3. Move the value into the entry with the new key.

This is not possible with the raw entry API. A RawOccupiedEntryMut mutably borrows the map so only one can exist at a time. A workaround is to compute the hash of the new key, do the lookup, destroy the raw entry and rebuild one with the hash later, but this is not ideal as it still does two lookups with the hash when one is sufficient.

RawOccupiedEntryMut already has insert_key that allows changing the key associated with an entry, but it does not actually reinserts the entry into the map, so if the new key has a different hash then the entry will no longer be accessible.

Add a reinsert method to reinsert the entry into the map after its key has been changed.

It is sometimes useful to modify the key associated with an entry
without changing the value. For example, when building a cache where
keys can change.

It is currently possible to implement this using hashbrown::HashMap by
removing the entry and adding a new one with the same value. However,
to prevent conflicts with the new key, the code has to:
1. Lookup the entry with the new key, and return an error if it
   occupied.
2. Delete the entry with the old key.
3. Move the value into the entry with the new key.

This is not possible with the raw entry API. A RawOccupiedEntryMut
mutably borrows the map so only one can exist at a time. A workaround is
to compute the hash of the new key, do the lookup, destroy the raw entry
and rebuild one with the hash later, but this is not ideal as it still
does two lookups with the hash when one is sufficient.

RawOccupiedEntryMut already has insert_key that allows changing the key
associated with an entry, but it does not actually reinserts the entry
into the map, so if the new key has a different hash then the entry will
no longer be accessible.

Add a reinsert method to reinsert the entry into the map after its key
has been changed.
@1kjo
Copy link
Author

1kjo commented Aug 3, 2023

The API is not ideal because when an entry cannot be reinserted because the map already contains an entry with the same key, it does not return a reference to that existing entry. This means that if the user wants to look at that existing entry, they have to do another lookup anyway later, which kinds of defeats the purpose of this API.

At least for my use case of using a hash map to implement a cache, it does not matter because when I modify the key for an entry, I expect the map not to have another entry with the same key.

I don't know how to fix it easily.

@Amanieu
Copy link
Member

Amanieu commented Aug 31, 2023

I would like to deprecate the raw entry API in favor of HashTable in #466. Could you have a look at that to see if it addresses your use case better?

@1kjo 1kjo mentioned this pull request Sep 4, 2023
@bors
Copy link
Collaborator

bors commented Sep 5, 2023

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

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.

None yet

3 participants