Skip to content

Disable inlining of packed io::Error destructor #149146

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −1
Open

Disable inlining of packed io::Error destructor #149146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion library/std/src/io/error/repr_bitpacked.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,13 @@ impl Repr {
}

impl Drop for Repr {
#[inline]
// This was #[inline] previously. Inlining the destructor of ErrorData is in
// no way helpful in real programs (as the source of the error will not be
// inlined, so there will not be any match assumptions to gain). The cost,
// meanwhile, is a code size increase by a factor of up to 5.4 in the case
// of dropping multiple io::Results in the same function
// (https://godbolt.org/z/8hfGchjsT).
#[inline(never)]
Copy link
Member

@scottmcm scottmcm Nov 21, 2025

Choose a reason for hiding this comment

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

Pondering: why is this the correct one to inline(never)? What if we made decode_repr be inline(never) and allowed inlining the destructor as that turns into just a call to decode_repr?

Said otherwise, if this drop is a problem, wouldn't data and data_mut and such also not want this inlining?

Copy link
Author

@kotauskas kotauskas Nov 21, 2025

Choose a reason for hiding this comment

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

decode_repr is a generic function in which C can be one of &Custom, &mut Custom, or Box<Custom>. In the case of the references, which are instantiated by data and data_mut, no types with destructors are involved and there are no destructors to shuffle around. In the Box<Custom> case, which is what's called by the destructor of Repr, decode_repr returns ownership of that which needs its destructor called and never calls any destructors itself, meaning that putting #[inline(never)] on it and removing it from Repr::drop would just make the destructor get inlined again (except the code bloat would be even worse because the branching would get duplicated).

Copy link
Contributor

@tgross35 tgross35 Nov 21, 2025

Choose a reason for hiding this comment

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

The ideal thing would be to ensure ErrorData's drop is outlined but only when it has a nontrivial drop, right? I was thinking this could be done with something like:

trait OutlineDrop { const SHOULD_OUTLINE: bool = false; }
impl<T> OutlineDrop for &T {}
impl<T> OutlineDrop for &mut T {}
impl OutlineDrop for Box<Custom> { const SHOULD_OUTLINE: bool = true; }

impl<C: MaybeOutlineDrop> Drop for ErrorData<C> {
    #[inline(always)]
    fn drop(&mut self) {
        #[inline(never)]
        fn drop_slow<C: MaybeOutlineDrop>(this: &mut ErrorData<C>) {
            // Insert a variant with no drop needed, then drop the custom variant
            // within this outlined function.
            let mut dst = ErrorData::Os(0);
            mem::swap(this, &mut dst);
            drop(dst);
        }

        // Only call the outlined drop
        if C::SHOULD_OUTLINE && matches!(self, ErrorData::Custom(_)) {
            drop_slow(self);
        }
    }
}

But from a quick check it seems like the matches! check is outlined, despite the #[inline(always)], which completely defeats the purpose.

The current optimization is kind of weird for this too. Looking at your godbolt link, it seems like it decides to inline the first drop and outline the second drop? Interesting heuristics.

Copy link
Contributor

@tgross35 tgross35 Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do think it's worth double checking whether there is an option to get this behavior in more places the nontrivial drop may happen, as Scott mentioned. But if there doesn't seem to be a good way, I don't see any reason not to make this change given the perf results.

What is the result without any #[inline]/#[inline(never)] btw? Seems like that could give it some flexibility.

fn drop(&mut self) {
// Safety: We're a Repr, decode_repr is fine. The `Box::from_raw` is
// safe because we're being dropped.
Expand Down
Loading