Skip to content

Commit ca06977

Browse files
committed
error: try to reduce amount of codegen by forcefully unlining error constructors
I had tried this several weeks ago, but recent exploration in #373 prompted me to revisit it. Indeed, `cargo llvm-lines` reveals that Jiff is emitting a _ton_ of code to LLVM just due to its error values. I don't know how much of this is inherent to Jiff's attention to good error messages or whether it's tied to how I've gone about it. With that said... This isn't a pareto distribution. It's not like Jiff's error handling is contributing to 80% of Jiff's LLVM lines or something. It's just the biggest chunk. But that chunk seems to be around 11% or so. This PR tries to reduce that chunk, but... doesn't succeed much. This PR almost certainly has perf regressions (haven't measured yet), but I wanted to put it up anyway to give folks an idea of what I'm trying. Also, here's the first ~30% of where Jiff is emitting LLVM lines (as run from the root of Jiff's repository): ``` Lines Copies Function name ----- ------ ------------- 203564 5977 (TOTAL) 10182 (5.0%, 5.0%) 86 (1.4%, 1.4%) <jiff::error::Error as jiff::error::ErrorContext>::with_context 5688 (2.8%, 7.8%) 204 (3.4%, 4.9%) core::result::Result<T,E>::map_err 3801 (1.9%, 9.7%) 145 (2.4%, 7.3%) <core::result::Result<T,E> as core::ops::try_trait::Try>::branch 2837 (1.4%, 11.1%) 116 (1.9%, 9.2%) core::option::Option<T>::ok_or_else 2120 (1.0%, 12.1%) 2 (0.0%, 9.3%) jiff::fmt::friendly::printer::SpanPrinter::print_duration_designators 1822 (0.9%, 13.0%) 2 (0.0%, 9.3%) jiff::fmt::temporal::printer::SpanPrinter::print_span 1658 (0.8%, 13.8%) 1 (0.0%, 9.3%) jiff::fmt::strtime::parse::Parser::parse 1652 (0.8%, 14.6%) 1 (0.0%, 9.3%) jiff::fmt::strtime::format::Formatter<W,L>::format_one 1548 (0.8%, 15.4%) 6 (0.1%, 9.4%) jiff::fmt::temporal::printer::DateTimePrinter::print_time 1312 (0.6%, 16.0%) 110 (1.8%, 11.3%) <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual 1260 (0.6%, 16.6%) 6 (0.1%, 11.4%) jiff::fmt::temporal::printer::DateTimePrinter::print_date 1219 (0.6%, 17.2%) 47 (0.8%, 12.1%) core::option::Option<T>::map 1160 (0.6%, 17.8%) 22 (0.4%, 12.5%) core::option::Option<T>::map_or 1071 (0.5%, 18.3%) 28 (0.5%, 13.0%) core::result::Result<T,E>::unwrap 997 (0.5%, 18.8%) 21 (0.4%, 13.3%) core::option::Option<T>::or_else 986 (0.5%, 19.3%) 1 (0.0%, 13.4%) jiff::span::Span::from_invariant_nanoseconds 976 (0.5%, 19.8%) 16 (0.3%, 13.6%) <jiff::util::rangeint::RangedDebug<_,_> as core::fmt::Debug>::fmt 920 (0.5%, 20.2%) 10 (0.2%, 13.8%) <jiff::util::rangeint::ri32<_,_> as jiff::util::rangeint::RFrom<jiff::util::rangeint::ri64<_,_>>>::rfrom 920 (0.5%, 20.7%) 4 (0.1%, 13.9%) jiff::fmt::temporal::printer::DateTimePrinter::print_offset_rounded 912 (0.4%, 21.1%) 6 (0.1%, 14.0%) jiff::fmt::friendly::printer::DesignatorWriter<W>::write 890 (0.4%, 21.6%) 2 (0.0%, 14.0%) jiff::fmt::temporal::printer::SpanPrinter::print_duration 840 (0.4%, 22.0%) 12 (0.2%, 14.2%) jiff::util::rangeint::ri8<_,_>::get 828 (0.4%, 22.4%) 6 (0.1%, 14.3%) core::slice::<impl [T]>::binary_search_by 819 (0.4%, 22.8%) 32 (0.5%, 14.8%) core::result::Result<T,E>::unwrap_or_else 802 (0.4%, 23.2%) 2 (0.0%, 14.9%) jiff::fmt::friendly::printer::SpanPrinter::print_span_hms 777 (0.4%, 23.6%) 10 (0.2%, 15.0%) jiff::util::rangeint::Composite<T>::map 758 (0.4%, 24.0%) 2 (0.0%, 15.1%) jiff::fmt::friendly::printer::SpanPrinter::print_duration_hms 744 (0.4%, 24.3%) 8 (0.1%, 15.2%) core::array::try_from_fn_erased 744 (0.4%, 24.7%) 2 (0.0%, 15.2%) jiff::fmt::temporal::printer::DateTimePrinter::print_pieces 720 (0.4%, 25.0%) 2 (0.0%, 15.3%) jiff::fmt::friendly::printer::SpanPrinter::print_span_designators_non_fraction 700 (0.3%, 25.4%) 10 (0.2%, 15.4%) <core::iter::adapters::zip::Zip<A,B> as core::iter::adapters::zip::ZipImpl<A,B>>::next 700 (0.3%, 25.7%) 7 (0.1%, 15.5%) <jiff::util::rangeint::ri8<_,_> as jiff::util::rangeint::RFrom<jiff::util::rangeint::ri64<_,_>>>::rfrom 682 (0.3%, 26.1%) 6 (0.1%, 15.6%) jiff::fmt::strtime::format::<impl jiff::fmt::strtime::Extension>::write_int 660 (0.3%, 26.4%) 10 (0.2%, 15.8%) <core::iter::adapters::enumerate::Enumerate<I> as core::iter::traits::iterator::Iterator>::next 659 (0.3%, 26.7%) 1 (0.0%, 15.8%) jiff::fmt::rfc2822::DateTimePrinter::print_civil_with_offset 600 (0.3%, 27.0%) 6 (0.1%, 15.9%) <jiff::util::rangeint::ri8<_,_> as jiff::util::rangeint::RFrom<jiff::util::rangeint::ri32<_,_>>>::rfrom 588 (0.3%, 27.3%) 12 (0.2%, 16.1%) jiff::util::rangeint::ri8<_,_>::try_new 588 (0.3%, 27.6%) 1 (0.0%, 16.1%) jiff::tz::db::zoneinfo::inner::walk 584 (0.3%, 27.9%) 86 (1.4%, 17.6%) <core::result::Result<T,jiff::error::Error> as jiff::error::ErrorContext>::with_context::{{closure}} 583 (0.3%, 28.2%) 1 (0.0%, 17.6%) jiff::civil::date::DateDifference::since_with_largest_unit 581 (0.3%, 28.4%) 11 (0.2%, 17.8%) jiff::error::RangeError::new 574 (0.3%, 28.7%) 2 (0.0%, 17.8%) core::slice::sort::stable::quicksort::stable_partition 552 (0.3%, 29.0%) 2 (0.0%, 17.9%) core::str::pattern::TwoWaySearcher::next_back 545 (0.3%, 29.3%) 97 (1.6%, 19.5%) <T as jiff::util::rangeint::RInto<U>>::rinto 544 (0.3%, 29.5%) 2 (0.0%, 19.5%) jiff::fmt::strtime::format::write_offset 536 (0.3%, 29.8%) 4 (0.1%, 19.6%) jiff::fmt::temporal::printer::DateTimePrinter::print_datetime ``` Other than error handling, it doesn't look like there is any one thing that is contributing the most here. That means that I think real improvements here are probably going to require combing through these and ensuring that the number of copies of each function is as small as it can be. In other words, I don't really see any low hanging fruit here, but maybe I'm not looking at this right. FWIW, this PR does decrease the number of LLVM lines, but only marginally: ``` Lines Copies Function name ----- ------ ------------- 195380 5768 (TOTAL) 3801 (1.9%, 1.9%) 145 (2.5%, 2.5%) <core::result::Result<T,E> as core::ops::try_trait::Try>::branch 3286 (1.7%, 3.6%) 86 (1.5%, 4.0%) <core::result::Result<T,jiff::error::Error> as jiff::error::ErrorContext>::with_context 2851 (1.5%, 5.1%) 96 (1.7%, 5.7%) core::result::Result<T,E>::map_err 2837 (1.5%, 6.5%) 116 (2.0%, 7.7%) core::option::Option<T>::ok_or_else 2120 (1.1%, 7.6%) 2 (0.0%, 7.7%) jiff::fmt::friendly::printer::SpanPrinter::print_duration_designators 1822 (0.9%, 8.6%) 2 (0.0%, 7.7%) jiff::fmt::temporal::printer::SpanPrinter::print_span 1658 (0.8%, 9.4%) 1 (0.0%, 7.8%) jiff::fmt::strtime::parse::Parser::parse 1652 (0.8%, 10.3%) 1 (0.0%, 7.8%) jiff::fmt::strtime::format::Formatter<W,L>::format_one 1548 (0.8%, 11.0%) 6 (0.1%, 7.9%) jiff::fmt::temporal::printer::DateTimePrinter::print_time 1312 (0.7%, 11.7%) 110 (1.9%, 9.8%) <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual 1260 (0.6%, 12.4%) 6 (0.1%, 9.9%) jiff::fmt::temporal::printer::DateTimePrinter::print_date 1219 (0.6%, 13.0%) 47 (0.8%, 10.7%) core::option::Option<T>::map 1160 (0.6%, 13.6%) 22 (0.4%, 11.1%) core::option::Option<T>::map_or 1071 (0.5%, 14.1%) 28 (0.5%, 11.6%) core::result::Result<T,E>::unwrap 997 (0.5%, 14.6%) 21 (0.4%, 11.9%) core::option::Option<T>::or_else 986 (0.5%, 15.1%) 1 (0.0%, 12.0%) jiff::span::Span::from_invariant_nanoseconds 976 (0.5%, 15.6%) 16 (0.3%, 12.2%) <jiff::util::rangeint::RangedDebug<_,_> as core::fmt::Debug>::fmt 925 (0.5%, 16.1%) 27 (0.5%, 12.7%) <core::result::Result<T,jiff::error::Error> as jiff::error::ErrorContext>::with_context::imp ``` I wonder if my approach to error handling with just using strings everywhere is hurting things. The main alternative would be structured errors everywhere. But there are _so many_ error messages that writing out structured definitions for each is terrifying to me (even if I was willing to use something like `thiserror` to reduce the boiler plate aspect of it). Ref #373
1 parent 08abead commit ca06977

1 file changed

Lines changed: 78 additions & 45 deletions

File tree

src/error.rs

Lines changed: 78 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -131,13 +131,41 @@ impl Error {
131131
pub fn from_args<'a>(message: core::fmt::Arguments<'a>) -> Error {
132132
Error::from(ErrorKind::Adhoc(AdhocError::from_args(message)))
133133
}
134+
135+
#[inline(never)]
136+
#[cold]
137+
fn context_impl(self, consequent: Error) -> Error {
138+
#[cfg(feature = "alloc")]
139+
{
140+
let mut err = consequent;
141+
if err.inner.is_none() {
142+
err = err!("unknown jiff error");
143+
}
144+
let inner = err.inner.as_mut().unwrap();
145+
assert!(
146+
inner.cause.is_none(),
147+
"cause of consequence must be `None`"
148+
);
149+
// OK because we just created this error so the Arc
150+
// has one reference.
151+
Arc::get_mut(inner).unwrap().cause = Some(self);
152+
err
153+
}
154+
#[cfg(not(feature = "alloc"))]
155+
{
156+
// We just completely drop `self`. :-(
157+
consequent
158+
}
159+
}
134160
}
135161

136162
impl Error {
137163
/// Creates a new "ad hoc" error value.
138164
///
139165
/// An ad hoc error value is just an opaque string.
140166
#[cfg(feature = "alloc")]
167+
#[inline(never)]
168+
#[cold]
141169
pub(crate) fn adhoc<'a>(message: impl core::fmt::Display + 'a) -> Error {
142170
Error::from(ErrorKind::Adhoc(AdhocError::from_display(message)))
143171
}
@@ -148,17 +176,31 @@ impl Error {
148176
/// `core::fmt::Arguments` down. This lets us extract a `&'static str`
149177
/// from some messages in core-only mode and provide somewhat decent error
150178
/// messages in some cases.
179+
#[inline(never)]
180+
#[cold]
151181
pub(crate) fn adhoc_from_args<'a>(
152182
message: core::fmt::Arguments<'a>,
153183
) -> Error {
154184
Error::from(ErrorKind::Adhoc(AdhocError::from_args(message)))
155185
}
156186

187+
/// Like `Error::adhoc`, but creates an error from a `String` directly.
188+
///
189+
/// This exists to explicitly monomorphize a very common case.
190+
#[cfg(feature = "alloc")]
191+
#[inline(never)]
192+
#[cold]
193+
fn adhoc_from_string(message: alloc::string::String) -> Error {
194+
Error::adhoc(message)
195+
}
196+
157197
/// Like `Error::adhoc`, but creates an error from a `&'static str`
158198
/// directly.
159199
///
160200
/// This is useful in contexts where you know you have a `&'static str`,
161201
/// and avoids relying on `alloc`-only routines like `Error::adhoc`.
202+
#[inline(never)]
203+
#[cold]
162204
pub(crate) fn adhoc_from_static_str(message: &'static str) -> Error {
163205
Error::from(ErrorKind::Adhoc(AdhocError::from_static_str(message)))
164206
}
@@ -167,6 +209,8 @@ impl Error {
167209
/// specified `min..=max` range. The given `what` label is used in the
168210
/// error message as a human readable description of what exactly is out
169211
/// of range. (e.g., "seconds")
212+
#[inline(never)]
213+
#[cold]
170214
pub(crate) fn range(
171215
what: &'static str,
172216
given: impl Into<i128>,
@@ -189,6 +233,8 @@ impl Error {
189233
///
190234
/// This is only available when the `std` feature is enabled.
191235
#[cfg(feature = "std")]
236+
#[inline(never)]
237+
#[cold]
192238
pub(crate) fn io(err: std::io::Error) -> Error {
193239
Error::from(ErrorKind::IO(IOError { err }))
194240
}
@@ -198,6 +244,8 @@ impl Error {
198244
/// This is a convenience routine for calling `Error::context` with a
199245
/// `FilePathError`.
200246
#[cfg(any(feature = "tzdb-zoneinfo", feature = "tzdb-concatenated"))]
247+
#[inline(never)]
248+
#[cold]
201249
pub(crate) fn path(self, path: impl Into<std::path::PathBuf>) -> Error {
202250
let err = Error::from(ErrorKind::FilePath(FilePathError {
203251
path: path.into(),
@@ -527,21 +575,24 @@ pub(crate) trait IntoError {
527575
}
528576

529577
impl IntoError for Error {
578+
#[inline(always)]
530579
fn into_error(self) -> Error {
531580
self
532581
}
533582
}
534583

535584
impl IntoError for &'static str {
585+
#[inline(always)]
536586
fn into_error(self) -> Error {
537587
Error::adhoc_from_static_str(self)
538588
}
539589
}
540590

541591
#[cfg(feature = "alloc")]
542592
impl IntoError for alloc::string::String {
593+
#[inline(always)]
543594
fn into_error(self) -> Error {
544-
Error::adhoc(self)
595+
Error::adhoc_from_string(self)
545596
}
546597
}
547598

@@ -581,70 +632,52 @@ pub(crate) trait ErrorContext {
581632
impl ErrorContext for Error {
582633
#[cfg_attr(feature = "perf-inline", inline(always))]
583634
fn context(self, consequent: impl IntoError) -> Error {
584-
#[cfg(feature = "alloc")]
585-
{
586-
let mut err = consequent.into_error();
587-
if err.inner.is_none() {
588-
err = err!("unknown jiff error");
589-
}
590-
let inner = err.inner.as_mut().unwrap();
591-
assert!(
592-
inner.cause.is_none(),
593-
"cause of consequence must be `None`"
594-
);
595-
// OK because we just created this error so the Arc
596-
// has one reference.
597-
Arc::get_mut(inner).unwrap().cause = Some(self);
598-
err
599-
}
600-
#[cfg(not(feature = "alloc"))]
601-
{
602-
// We just completely drop `self`. :-(
603-
consequent.into_error()
604-
}
635+
self.context_impl(consequent.into_error())
605636
}
606637

607638
#[cfg_attr(feature = "perf-inline", inline(always))]
608639
fn with_context<E: IntoError>(
609640
self,
610641
consequent: impl FnOnce() -> E,
611642
) -> Error {
612-
#[cfg(feature = "alloc")]
613-
{
614-
let mut err = consequent().into_error();
615-
if err.inner.is_none() {
616-
err = err!("unknown jiff error");
617-
}
618-
let inner = err.inner.as_mut().unwrap();
619-
assert!(
620-
inner.cause.is_none(),
621-
"cause of consequence must be `None`"
622-
);
623-
// OK because we just created this error so the Arc
624-
// has one reference.
625-
Arc::get_mut(inner).unwrap().cause = Some(self);
626-
err
627-
}
628-
#[cfg(not(feature = "alloc"))]
629-
{
630-
// We just completely drop `self`. :-(
631-
consequent().into_error()
632-
}
643+
self.context_impl(consequent().into_error())
633644
}
634645
}
635646

636647
impl<T> ErrorContext for Result<T, Error> {
637648
#[cfg_attr(feature = "perf-inline", inline(always))]
638649
fn context(self, consequent: impl IntoError) -> Result<T, Error> {
639-
self.map_err(|err| err.context(consequent))
650+
#[inline(never)]
651+
#[cold]
652+
fn imp<U>(
653+
this: Result<U, Error>,
654+
consequent: Error,
655+
) -> Result<U, Error> {
656+
match this {
657+
Ok(value) => Ok(value),
658+
Err(err) => Err(err.context_impl(consequent)),
659+
}
660+
}
661+
imp(self, consequent.into_error())
640662
}
641663

642664
#[cfg_attr(feature = "perf-inline", inline(always))]
643665
fn with_context<E: IntoError>(
644666
self,
645667
consequent: impl FnOnce() -> E,
646668
) -> Result<T, Error> {
647-
self.map_err(|err| err.with_context(consequent))
669+
#[inline(never)]
670+
#[cold]
671+
fn imp<U>(
672+
this: Result<U, Error>,
673+
consequent: Error,
674+
) -> Result<U, Error> {
675+
match this {
676+
Ok(value) => Ok(value),
677+
Err(err) => Err(err.context_impl(consequent)),
678+
}
679+
}
680+
imp(self, consequent().into_error())
648681
}
649682
}
650683

0 commit comments

Comments
 (0)