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

Updating to hashbrown 0.12.0 #217

Closed
erickt opened this issue Feb 4, 2022 · 5 comments
Closed

Updating to hashbrown 0.12.0 #217

erickt opened this issue Feb 4, 2022 · 5 comments

Comments

@erickt
Copy link
Contributor

erickt commented Feb 4, 2022

Would it be possible to update the hashbrown dependency to 0.12.0? We're going through an audit of indexmap and hashbrown, and we noticed that in hashbrown 0.12.0 they marked RawTable::insert_no_grow as unsafe: rust-lang/hashbrown#254.

I made an attempt at doing an update, but I'm having trouble telling how erase_indices and rebuild_hash_table guarantee that the indices table has enough space to use insert_no_grow. Do you know what maintains these invariants? Or should we just switch those locations to use RawTable::insert?

@cuviper
Copy link
Member

cuviper commented Feb 7, 2022

Would it be possible to update the hashbrown dependency to 0.12.0?

I hesitate to do this too soon because it bumps MSRV to 1.56, and we already have folks complaining about 1.49 (#214).

we noticed that in hashbrown 0.12.0 they marked RawTable::insert_no_grow as unsafe: rust-lang/hashbrown#254.

Yes, that was me. :) AFAIK our current usage already meets those safety conditions anyway, so there shouldn't be any functional difference for us between 0.11 and 0.12, but I welcome you to confirm that independently.

I made an attempt at doing an update, but I'm having trouble telling how erase_indices and rebuild_hash_table guarantee that the indices table has enough space to use insert_no_grow. Do you know what maintains these invariants?

Both of these are conditions where we just cleared the table and then we are reinserting the same or fewer entries. If there's any question whether that's actually true, I suppose we could add a runtime assert! to make sure of it. A single check shouldn't affect performance much, versus repeated insert checks and all its extra codegen for resizing that we shouldn't need.

@paolobarbolini
Copy link

I hesitate to do this too soon because it bumps MSRV to 1.56, and we already have folks complaining about 1.49 (#214).

Could we reconsider this now that 1.56 is 6 months old?

@cuviper
Copy link
Member

cuviper commented Apr 15, 2022

Well, I already upgraded on master in preparation for v2, #219. I think we need to decide on #147 though before release, because adding generic parameters will break some uses, IIRC, even with a default. I'd like #177 too, but there's nothing breaking there.

@tustvold
Copy link

Rust 1.56 has now been out for the best part of 8 months now, are there any plans to release a 1.x version that bumps the dependency?

@cuviper
Copy link
Member

cuviper commented Jun 17, 2022

I had no plans for 1.x, but we can do it -- see #231.

@cuviper cuviper closed this as completed Jun 17, 2022
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

No branches or pull requests

4 participants