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

BTree: let method clear recycle root memory #94800

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Mar 10, 2022

BTree's clear methods up to now literally drops and starts anew. HashMap's clear "keeps the allocated memory for reuse.". This PR tries the simplest way to preserve the maximum of memory. If a map/set has multiple nodes, clear doesn't recycle the original root node but the last one in the tree, which is the last leaf node that was deallocated up to now and which would most likely have been the same memory allocated once you add elements to the cleared map.

This is a regression for those wanting to deallocate as much memory as possible. They could use my_map = BTreeMap::new() instead. Is there a regression in normal drop or into_iter? I have no idea.

Also, now two key-value pairs are allowed to panic-on-drop and unwinding continues, an abort only happens on the third pair panicking. This can be remedied, but I assumed it's acceptable behaviour.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022
@ssomers ssomers force-pushed the btree_clever_clear branch 2 times, most recently from 55ce439 to 0d3a80c Compare March 12, 2022 17:46
@kennytm kennytm added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 13, 2022
@bors
Copy link
Contributor

bors commented Mar 20, 2022

☔ The latest upstream changes (presumably #92962) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022
@bors
Copy link
Contributor

bors commented Jun 16, 2022

☔ The latest upstream changes (presumably #98103) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jun 18, 2022

☔ The latest upstream changes (presumably #98178) made this pull request unmergeable. Please resolve the merge conflicts.

@ssomers ssomers closed this Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants