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

Prepare hashbrown for inclusion in the standard library #46

Merged
merged 4 commits into from
Mar 30, 2019
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Feb 25, 2019

src/raw/mod.rs Outdated Show resolved Hide resolved
@Gankra
Copy link

Gankra commented Mar 6, 2019

Ok here's my notes from doing a fairly quick skim over map.rs (since it's a pretty trivial file now):

general style nit: it's a bit more clear (and reads less goofy) to write T::default() than Default::default()

general style nit: in case you missed it, you can now write fn iter(&self) -> Iter<'_, K, V> to make elided lifetimes explicit. I'm not sure how idiomatic or desirable this is. Just felt like mentionning it. (I'm personally super mixed on it)

general style nit: it's possible also struct Iter<'a, K: 'a, V:'a> can elide the : 'a's now, although I'm not sure if the unsafe guts break inference here. shrug

performance concern on insert: inserting a new key performs a double-lookup. I am very surprised that you considered this acceptable? Why not write insert in terms of entry?

performance concern on VacantEntry: Oh Wait This Has The Same Issue? Literally the core motivation of this API is avoiding this kind of double-lookup. I suppose the argument is that lookup is so fast that redoing it on insert is fine, in conjunction with the fact find/insert want fundamentally different matching algorithms..?

performance/correctness concern on RawVacantEntryMut::insert: I don't think you're supposed to rehash the key here. I believe the intended contract of the API is that we trust you to pass a key with a matching hash (which we already computed to get here).

@Amanieu
Copy link
Member Author

Amanieu commented Mar 6, 2019

Regarding the concerns about double-lookup, this is by design. SInce SwissTable uses tombstones, the first empty bucket that you find is not necessarily the one you will insert into: you could reuse a tombstone.

This is why normal lookups use match_empty to determine whether to end the scan, while insertions use match_empty_or_deleted to find a slot to insert into.

Note that VacantEntry still helps avoid the initial lookup on insertion that checks whether the map already contains the given key, so it is not completely pointless.

@Gankra
Copy link

Gankra commented Mar 6, 2019

That makes sense. And also explains why you wouldn't want to at least cache which group you stopped on to resume insertion (you could have walked over an massive graveyard of DELETED to find an empty slot).

@Amanieu
Copy link
Member Author

Amanieu commented Mar 6, 2019

Regarding RawVacentEntryMut::insert, that's what the insert_hashed_nocheck method is for.

@Gankra
Copy link

Gankra commented Mar 7, 2019

hrm, this seems like a really whack API design, since one of the biggest motivations was "lookup with &K -> insert K", which this pair of methods is specifically making awkward to do optimally. But if what the libs folks settled on... shrug

@Amanieu
Copy link
Member Author

Amanieu commented Mar 8, 2019

Since the new Cargo.toml requires rename-dependencies which is only available in 1.31, I went ahead and converted the entire crate to 2018 edition.

This is technically a breaking change since the minimum Rust version is bumped from 1.29 to 1.31.

Also adds PhantomData<T> to RawTable and RawIntoIter
@Amanieu
Copy link
Member Author

Amanieu commented Mar 30, 2019

bors r+

bors bot added a commit that referenced this pull request Mar 30, 2019
46: Prepare hashbrown for inclusion in the standard library r=Amanieu a=Amanieu

See rust-lang/rust#58623.

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 30, 2019

Build failed

  • continuous-integration/travis-ci/push

@Amanieu
Copy link
Member Author

Amanieu commented Mar 30, 2019

bors retry

bors bot added a commit that referenced this pull request Mar 30, 2019
46: Prepare hashbrown for inclusion in the standard library r=Amanieu a=Amanieu

See rust-lang/rust#58623.

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 30, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit f9bcc42 into master Mar 30, 2019
@Amanieu Amanieu deleted the libstd branch April 24, 2019 19:20
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.

2 participants