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

Remove implicit shrinking from hash_map. #18770

Merged
merged 2 commits into from
Dec 4, 2014

Conversation

pczarn
Copy link
Contributor

@pczarn pczarn commented Nov 8, 2014

Part of enforcing capacity-related conventions, for #18424, the collections reform.

Implements fn shrink_to_fit for HashMap.
The reserve method now takes as an argument the extra space to reserve.

@huonw
Copy link
Member

huonw commented Nov 8, 2014

Preferably the reserve method change would be a in separate commit with a [breaking-change] marker.

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch 2 times, most recently from f0d54cf to 3bdd519 Compare November 8, 2014 13:06
@pczarn
Copy link
Contributor Author

pczarn commented Nov 8, 2014

Done. It's a little unintuitive that a breaking-change probably won't break any code.

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch from 3bdd519 to ef75c37 Compare November 8, 2014 13:07
fn capacity_range(&self, new_size: uint) -> (uint, uint) {
// Here, we are rephrasing the logic by specifying the ranges:
fn min_capacity(&self, new_size: uint) -> uint {
// Here, we are rephrasing the logic by specifying the range:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just drop "the range" here

@Gankra
Copy link
Contributor

Gankra commented Nov 8, 2014

All the actual implementation logic looks good to me. Just some doc nits, and some issues about the precision we can assume for resize in testing.

@Gankra
Copy link
Contributor

Gankra commented Nov 8, 2014

Oh! Hashset's capacity stuff is probably all wrong now. Probably best to fix that up in this PR too? (should just be doc, maybe test changes)

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch 3 times, most recently from dc7c5cf to 31ec9ee Compare November 8, 2014 16:06
@Gankra
Copy link
Contributor

Gankra commented Nov 8, 2014

This all looks basically good to me now. Just need a real reviewer to take a look. Might want to update some tests to reflect newer semantics but it's not a big deal. Also might want to wait for @aturon to make a proclamation about how we want to map capacity() to internal vs usable capacity.

r? @huonw

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch 2 times, most recently from e6da055 to f10cac8 Compare November 8, 2014 16:31
@huonw
Copy link
Member

huonw commented Nov 9, 2014

It's a little unintuitive that a breaking-change probably won't break any code.

This is breaking code: it will not stop things from compiling, but it is dramatically changing semantics and could, e.g. cause code that previously worked fine to run out of memory (map.reserve(x) becomes equivalent to map.reserve(map.len() + x) in the old code).

let cap = num::next_power_of_two(
max(INITIAL_CAPACITY, new_minimum_capacity));
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn reserve(&mut self, additional: uint) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle arithmetic overflow correctly?

@aturon
Copy link
Member

aturon commented Nov 9, 2014

@gankro

Also might want to wait for @aturon to make a proclamation about how we want to map capacity() to internal vs usable capacity.

Usable capacity, for sure. Everything else is an implementation detail that should be hidden. If we ever want to be able to use capacity and reserve generically in the future, this is the right way to go IMO.

@pczarn
Copy link
Contributor Author

pczarn commented Nov 9, 2014

Updated with usable capacity

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch from a0764ef to 2f4f753 Compare November 17, 2014 13:25
@Gankra
Copy link
Contributor

Gankra commented Nov 18, 2014

Whoops, this slipped through the cracks. I believe all of my issues were addressed. @huonw anything outstanding for you?

fn reserve(&mut self, new_capacity: uint) {
self.minimum_capacity2 = new_capacity << 1;
fn usable_capacity(&self, cap: uint) -> uint {
// Integer division with rounding is inexact. However,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the required property just min_capacity(usable_capacity(x)) <= x? Because this is always true for positive x: integer division is floored, not just some unspecified rounding, i.e. (cap / 11) * 11 <= cap always.

(I'm just raising this because that property means this comment is slightly confusing to me, as it is either redundant or talking about something else entirely.)

@pczarn
Copy link
Contributor Author

pczarn commented Nov 21, 2014

Done. Hopefully I got capacity equality right.

@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch 3 times, most recently from 11dc54e to 7564168 Compare November 29, 2014 20:09
Implements fn shrink_to_fit for HashMap.
HashMap's `reserve` method now takes as an argument the *extra* space
to reserve.

[breaking-change]
@pczarn pczarn force-pushed the hash_map-explicit-shrinking branch from 7564168 to b82624b Compare November 30, 2014 21:52
bors added a commit that referenced this pull request Dec 4, 2014
Part of enforcing capacity-related conventions, for #18424, the collections reform.

Implements `fn shrink_to_fit` for HashMap.
The `reserve` method now takes as an argument the *extra* space to reserve.
@bors bors closed this Dec 4, 2014
@bors bors merged commit b82624b into rust-lang:master Dec 4, 2014
@pczarn pczarn deleted the hash_map-explicit-shrinking branch January 14, 2015 16:55
lnicola added a commit to lnicola/rust that referenced this pull request Jan 7, 2025
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