Skip to content

Commit 719220c

Browse files
committed
Auto merge of #149146 - kotauskas:patch-1, r=<try>
Disable inlining of custom `io::Error` destructor
2 parents 54333ff + 7237890 commit 719220c

1 file changed

Lines changed: 45 additions & 8 deletions

File tree

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

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -226,20 +226,57 @@ 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.
233-
unsafe {
234-
let _ = decode_repr(self.0, |p| Box::<Custom>::from_raw(p));
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+
unsafe fn drop_custom(ptr: *mut ()) {
254+
unsafe {
255+
// SAFETY: we're being dropped
256+
let _ = Box::from_raw(ptr.byte_sub(TAG_CUSTOM).cast::<Custom>());
257+
}
258+
}
259+
// The sum of the below code and the body of drop_custom is a
260+
// transclusion from decode_repr. This used to be a call to
261+
// decode_repr, but we need the amount of IR produced here to be the
262+
// smallest it can possibly be, as this destructor is ubiquitously
263+
// inlined into every io::Error destruction site. It has been confirmed
264+
// via visual inspection of the disassembly that the codegen for that
265+
// was absolutely atrocious. As such, the duplication of logic is
266+
// justified.
267+
let ptr = self.0.as_ptr();
268+
if ptr.addr() & TAG_MASK == TAG_CUSTOM {
269+
// SAFETY: as per decode_repr
270+
unsafe { drop_custom(ptr) };
235271
}
236272
}
237273
}
238274

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`)
275+
/// Shared helper to decode a [`Repr`]'s internal pointer into an [`ErrorData`].
276+
///
277+
/// # Safety
278+
/// `ptr`'s bits should be encoded as described in the document at the
279+
/// top (it should be `some_repr.0`).
243280
#[inline]
244281
unsafe fn decode_repr<C, F>(ptr: NonNull<()>, make_custom: F) -> ErrorData<C>
245282
where

0 commit comments

Comments
 (0)