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

Improve RawIter re-usability #175

Merged
merged 13 commits into from Jul 4, 2020
Merged

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jul 3, 2020

Creating a new iterator for a table is occasionally expensive,
especially if a suffix of the table (since we iterate "backwards") is
sparsely populated. This PR adds methods that allow re-using an existing
RawIter in more situations.

Replaces #167.

This allows holders of a `RawIter` to continue using that iterator even
if they perform concurrent removals on the underlying table.
src/raw/mod.rs Outdated Show resolved Hide resolved
src/raw/mod.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Jul 3, 2020

I like this much better!

jonhoo added a commit to jonhoo/griddle that referenced this pull request Jul 3, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 3, 2020

Glad to hear it! I have now also implemented the necessary changes in griddle, and it seems to be sufficient: jonhoo/griddle#12. The Clone implementation isn't as pretty as I'd like it to be, but it's not too bad.

The griddle test suite also passes, including a number of additional iterations of the quickcheck tests I've run locally, so I think this is ready now 🎉

@jonhoo jonhoo marked this pull request as ready for review July 3, 2020 17:42
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 3, 2020

One really awkward side-effect of this (and on jonhoo/griddle#12 in particular) is that griddle's HashMap now cannot implement Clone without K: Hash, S: BuildHasher bounds, since it must perform inserts. This, in turn, means that derive(Clone) will no longer work 😢 griddle::raw::RawTable can't be Clone at all, since it requires a hasher.

@Amanieu
Copy link
Member

Amanieu commented Jul 3, 2020

Clone probably should depend on K: Hash, S: BuildHasher. The only reason hashbrown doesn't is because it needs to be backward compatible with the libstd HashMap which doesn't have those bounds on its Clone impl.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 3, 2020

Is there anything more you'd like to see added to this PR? I've tried to document as I went, and have also added the appropriate "resume from RawIter" methods for drain and into_iter.

@Amanieu Amanieu merged commit f3fd89d into rust-lang:master Jul 4, 2020
@Amanieu
Copy link
Member

Amanieu commented Jul 4, 2020

Thanks!

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

2 participants