Skip to content

Make dropping an empty BTreeMap free #161791

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.

Sign up for GitHub

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

Merged
rust-bors merged 7 commits into from
Sep 26, 2026
Merged

Make dropping an empty BTreeMap free #161791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0109ed2
BTree: make dropping an empty BTreeMap free
jakedrew Sep 22, 2026
756abe8
Adding codegen for the empty BTree drop
jakedrew Sep 22, 2026
15831b0
BTree: documenting the unsafe in the empty map drop
jakedrew Sep 22, 2026
70ff620
Check for dealloc instead of dying_next
jakedrew Sep 22, 2026
4cf035f
Checking that an empty BTreeMap drop compiles down to only a `ret void`,
jakedrew Sep 23, 2026
9c77e1e
Adding safety comment on the unsafe in BTreeMap drop
jakedrew Sep 23, 2026
d432494
Comment that `dying_next` is too costly to inline
jakedrew Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions library/alloc/src/collections/btree/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,17 @@ pub struct BTreeMap<
#[stable(feature = "btree_drop", since = "1.7.0")]
unsafe impl<#[may_dangle] K, #[may_dangle] V, A: AllocatorClone> Drop for BTreeMap<K, V, A> {
fn drop(&mut self) {
// ignore-tidy-undocumented-unsafe
drop(unsafe { ptr::read(self) }.into_iter())
// Skip `into_iter` for an empty map: `dying_next` is too costly to inline, so the
// empty drop isn't optimised away (see #161375).
if self.root.is_some() {
// SAFETY: `self` is not used after this and none of its fields are dropped again:
// `alloc` is `ManuallyDrop` and `root` has no drop glue.
drop(unsafe { ptr::read(self) }.into_iter())
Comment on lines +211 to +213

@clarfonthey clarfonthey Sep 25, 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 guess that part of my confusion here is why this isn't sufficiently optimized into a no-op for an empty tree, and hence needed at all.

I figure it makes sense just due to the combined complexity of the function, but is there maybe other detail we could add here to make that clear, so maybe this could be fixed later?

View changes since the review

@jakedrew jakedrew Sep 25, 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.

Yes so at -Copt-level=3 inlining doesn’t occur because it’s too costly to inline (cost=850, threshold=250), and setting inline-threshold to 2000 does inline to just ret void.

I’ll add comment to explain this.

@clarfonthey clarfonthey Sep 25, 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.

Would inline(always) on some of the internal methods make the difference?

Mostly trying to see if we can potentially circumvent the need for this.

@jakedrew jakedrew Sep 25, 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.

Adding inline(always) on dying_next does fix the empty drop without the root check.

But the catch is that IntoIter::next uses dying_next too, so it gets inlined there as well.

With a small test:

use std::collections::BTreeMap;
#[no_mangle]
pub fn sum(m: BTreeMap<u64, u64>) -> u64 { 
    m.into_iter().map(|(k, v)| k + v).sum() 
}

At -Copt-level=3 this function grows from 165 to 429 lines of IR.

@clarfonthey clarfonthey Sep 26, 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.

Yup, seems definitely not worth it. Thank you for the extra research; I figured this was the right choice, just, wanted to make extra sure.

} else {
// SAFETY: With no root there are no nodes to free, so only the allocator needs
// dropping. `self` is not used after this, and `alloc` is dropped only here.
unsafe { ManuallyDrop::drop(&mut self.alloc) }
}
}
}

Expand Down
15 changes: 15 additions & 0 deletions tests/codegen-llvm/btree-empty-drop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Regression test for https://github.com/rust-lang/rust/issues/161375
// Checking that an empty BTree's drop is optimized away.
//@ compile-flags: -Copt-level=3

#![crate_type = "lib"]

use std::collections::BTreeMap;

// CHECK-LABEL: @drop_btree(
// CHECK-NEXT: {{.*}}:
// CHECK-NEXT: ret void
#[no_mangle]
pub fn drop_btree() {
let _ = BTreeMap::<(), ()>::new();
}
Loading