Skip to content

hash_map::OccupiedEntry::remove_entry #20601

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

Closed
wants to merge 1 commit into from

Conversation

stepancheg
Copy link
Contributor

remove_entry is like to remove, but returns both key and value.

Sometimes key needs to be recovered, for example, when lookup key
is different from storage key.

P. S. HashSet::remove(..) is stable now, so it can't be changed to return contained value. What do you think about having another operation like remove_value. I don't need it personally, but it should probably exist for consistency and flexibility.

P. P. S. I need remove_entry operation to implement LinkedHashSet and LinkedHashMap.

`remove_entry` is like to `remove`, but returns both key and value.

Sometimes key needs to be recovered, for example, when lookup key
is different from storage key.
@rust-highfive
Copy link
Contributor

r? @brson

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

@Gankra
Copy link
Contributor

Gankra commented Jan 6, 2015

Some previous discussion: http://discuss.rust-lang.org/t/maps-sets-and-the-value-of-keys/532

The overwhelming sentiment I've received is that keys are not sufficiently interesting (most collections APIs I've seen ignore them).

That said, LruCache (which has been moved to collect-rs), is largely just a LinkedHashMap implementation; why doesn't what it does work for you?

CC @aturon

@stepancheg
Copy link
Contributor Author

@gankro yes, LruCache is almost what I need. Thanks. I think LinkedHashMap can be extracted from LruCache, and LruCache made a thin wrapper of LinkedHashMap.

Anyway, remove_entry allows slightly more efficient implementation of LruCache: map field could be just map: HashMap<Box<LruEntry<K, V>>, ()>.

@Gankra
Copy link
Contributor

Gankra commented Jan 6, 2015

I'd be interested to see actual performance numbers, if possible.

@stepancheg
Copy link
Contributor Author

@gankro I doubt LruCache with remove_entry would be significantly faster, but it would occupy less space.

My speculation:

Assuming LruCache of type <u32, u32>.
Size of LruEntry is 8+8+4+4 = 24.
Size of Box<LruEntry> is 24 + malloc overhead + size of pointer = 40 bytes.
Size of KeyRef is 8.
Size of entry in hashtable is size of key + size of value + size of hash = 8 + 40 + 8 = 56 bytes.

When there's no KeyRef, size of entry in hashtable would be: 40 + 8 = 48 bytes.

Also, note, that hash map load factor is 90%.

So with remove_key LruCache would occupy about 10% less space.

@frankmcsherry
Copy link
Contributor

Just going to comment that, unless I misunderstand, another use case is when the keys are basically identical, but contain owned resources. If I have a Vec in my key (I do, some times), I'd may need to recover that Vec when I remove the entry. I could just clone the key that I used, of course, so it's not the end of the world, but it is a bit silly that the HashMap is just sitting on exactly the resources I need. By "silly" I mean "involves a conversation with the allocator". :)

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

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.

6 participants