Skip to content

Make dropping an empty BTreeMap free - #161791

Open
jakedrew wants to merge 6 commits into
rust-lang:mainfrom
jakedrew:fix/Making_and_dropping_an_empty_BTreeMap_is_not_free_161375
Open

Make dropping an empty BTreeMap free#161791
jakedrew wants to merge 6 commits into
rust-lang:mainfrom
jakedrew:fix/Making_and_dropping_an_empty_BTreeMap_is_not_free_161375

Conversation

@jakedrew

@jakedrew jakedrew commented Aug 26, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

#161375 (code example) shows that drop of an empty BTree map walks the dying_next path and so is not free. I tracked this down to 7b28036 in 1.56.0 using godbolt’s version comparison.

When the Btree map has no root we use the ManuallyDrop to safely drop the allocator early resulting in a free drop and a ret instead of the dying_next path.

Includes codegen test which correctly catches the prior dying_next case, which I based on uninit-aggregate-field.rs.

Fixes #161375

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2026
@rustbot

rustbot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

r? @mati865

rustbot has assigned @mati865.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 75 candidates
  • Random selection from 17 candidates

@jakedrew jakedrew changed the title Fix/making and dropping an empty b tree map is not free 161375 Make dropping an empty BTreeMap free Aug 26, 2026
@mati865

mati865 commented Aug 26, 2026

Copy link
Copy Markdown
Member

Nice, but that requires libs review. r? rust-lang/libs

@rustbot rustbot assigned clarfonthey and unassigned mati865 Aug 26, 2026
@ruriww

ruriww commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Just a side note, but can't the unsafe code be replaced with take(self)?

Hmm, maybe not https://godbolt.org/z/Wzhqh9jre.

clarfonthey
clarfonthey reviewed Aug 27, 2026
View reviewed changes
Comment thread tests/codegen-llvm/btree-empty-drop.rs Outdated
use std::collections::BTreeMap;

// CHECK-LABEL: @drop_btree
// CHECK-NOT: dying_next

@clarfonthey clarfonthey Aug 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally would rather this check for calls to dealloc rather than dying_next, since think I this test could easily become worthless if this method is renamed.

View changes since the review

@jakedrew jakedrew Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I switched to dealloc and then tried it on the old and new code and both passed. So I've switched to just checking that the function is only ret void which does catch this regression

@clarfonthey clarfonthey Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you, I was actually going to also recommend verifying that it fails the previous code, and you're already ahead of that.

@clarfonthey

clarfonthey commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2026
@rustbot

rustbot commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-bors

This comment has been minimized.

Sign in to view
@jakedrew
jakedrew force-pushed the fix/Making_and_dropping_an_empty_BTreeMap_is_not_free_161375 branch from e93ed22 to 70ff620 Compare September 22, 2026 23:15
@rustbot

rustbot commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@jakedrew
Checking that an empty BTreeMap drop compiles down to only a ret void,
4cf035f 
and that function only match drop_btree exactly. As the previous dealloc
also passed on old code so didn't catch the regression
clarfonthey
clarfonthey reviewed Sep 23, 2026
View reviewed changes
Comment thread library/alloc/src/collections/btree/map.rs Outdated
// ignore-tidy-undocumented-unsafe
drop(unsafe { ptr::read(self) }.into_iter())
if self.root.is_some() {
// ignore-tidy-undocumented-unsafe

@clarfonthey clarfonthey Sep 23, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would also prefer to update any missing safety comments when they're relevant.

View changes since the review

@jakedrew jakedrew Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Have added a comment now

@jakedrew

jakedrew commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 24, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clarfonthey clarfonthey clarfonthey left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@clarfonthey clarfonthey

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Making and dropping an empty BTreeMap is not free.

5 participants

@jakedrew @rustbot @mati865 @ruriww @clarfonthey