Skip to content

Commit e7abc97

Browse files
committed
Auto merge of #149146 - kotauskas:patch-1, r=<try>
Disable inlining of custom `io::Error` destructor
2 parents b52edc2 + 4298a05 commit e7abc97

1 file changed

Lines changed: 40 additions & 8 deletions

File tree

library/std/src/io/error/repr_bitpacked.rs

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -226,21 +226,53 @@ impl Repr {
226226
}
227227

228228
impl Drop for Repr {
229+
// What this will end up inlining into the destructor call site is merely a
230+
// bitwise AND of the pointer with the constant TAG_MASK, a comparison to
231+
// the constant TAG_CUSTOM, and a call to drop_custom that is jumped over
232+
// if the tag isn't TAG_CUSTOM.
229233
#[inline]
230234
fn drop(&mut self) {
231-
// Safety: We're a Repr, decode_repr is fine. The `Box::from_raw` is
232-
// safe because we're being dropped.
235+
// Inlining the destructor of a custom error does not confer any
236+
// optimization opportunities, as it is type-erased. The cost,
237+
// meanwhile, is a code size increase by a factor of up to 5.4 in the
238+
// case of dropping multiple io::Results in the same function
239+
// (https://godbolt.org/z/8hfGchjsT).
240+
//
241+
// FIXME the 32-bit variant still has the code bloat that is being
242+
// avoided here, see the discussion in
243+
// https://github.com/rust-lang/rust/pull/149146
244+
#[inline(never)]
245+
// The destructor of a custom error value is normally only called
246+
// after it has been displayed or logged, or if it has been downcasted
247+
// and handled appropriately. Displaying and logging errors is
248+
// considered a cold path in most programs, and downcasting is
249+
// similarly not something that is considered performance-critical.
250+
// Thus, in the interest of happy path performance, have block
251+
// placement get the calls to this function out of the way.
252+
#[cold]
253+
fn drop_custom(ptr: *mut Custom) {
254+
unsafe {
255+
// SAFETY: we're being dropped
256+
let _ = Box::from_raw(ptr);
257+
}
258+
}
233259
unsafe {
234-
let _ = decode_repr(self.0, |p| Box::<Custom>::from_raw(p));
260+
// SAFETY: we're a Repr
261+
let _ = decode_repr(self.0, drop_custom);
235262
}
236263
}
237264
}
238265

239-
// Shared helper to decode a `Repr`'s internal pointer into an ErrorData.
240-
//
241-
// Safety: `ptr`'s bits should be encoded as described in the document at the
242-
// top (it should `some_repr.0`)
243-
#[inline]
266+
/// Shared helper to decode a [`Repr`]'s internal pointer into an [`ErrorData`].
267+
///
268+
/// # Safety
269+
/// `ptr`'s bits should be encoded as described in the document at the
270+
/// top (it should be `some_repr.0`).
271+
// Allow outlining only if that debug assert below is actually there. We
272+
// don't say inline(never) for the second case here because people sometimes
273+
// enable debug assertions in release builds.
274+
#[cfg_attr(not(debug_assertions), inline(always))]
275+
#[cfg_attr(debug_assertions, inline)]
244276
unsafe fn decode_repr<C, F>(ptr: NonNull<()>, make_custom: F) -> ErrorData<C>
245277
where
246278
F: FnOnce(*mut Custom) -> C,

0 commit comments

Comments
 (0)