Skip to content
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.

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

Make DelayedFormat hold a generic offset #1559

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

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Apr 6, 2024

Just learned that adding a type parameter is a backwards-compatible change if it has a default type, thanks to RFC 0213.

That means we can fix the biggest remaining performance problem with DelayedFormat, and make our formatting work without allocations! 🎉

In this PR I only added an extra generic parameter to DelayedFormat. Making our formatting work without allocating needs one more tricky thing that is best for another PR.

To quote from #1163:

Our current formatting code has noticeable overhead (see #94). This is mostly caused by one design choice: if DelayedFormat::new is given an offset, it first formats a timezone name into a String. In case of DateTime<FixedOffset> and DateTime<Local> this involves formatting the offset, as there is no name available. This is responsible for 20~25% of the ca. 40% overhead in the format_with_items benchmark.

By taking the offset as a generic we can delay formatting the timezone name until it is needed, which often is never.

To keep the deprecated format::{format, format_items} functions working (which expect a String and FixedOffset instead of a generic offset) I had to add an OffsetWrapper type.

Benchmarks

     Running benches/chrono.rs (target/release/deps/chrono-413c84d256ba35f5)
bench_format            time:   [467.24 ns 470.63 ns 475.26 ns]
                        change: [-16.270% -15.552% -14.816%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  5 (5.00%) high mild
  5 (5.00%) high severe

bench_format_with_items time:   [323.83 ns 325.30 ns 326.94 ns]
                        change: [-18.296% -17.524% -16.769%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 65.85366% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 91.77%. Comparing base (0cfc405) to head (30735ee).

Files Patch % Lines
src/format/formatting.rs 69.69% 10 Missing ⚠️
src/date.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1559      +/-   ##
==========================================
- Coverage   91.80%   91.77%   -0.04%     
==========================================
  Files          37       37              
  Lines       18148    18149       +1     
==========================================
- Hits        16661    16656       -5     
- Misses       1487     1493       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

where
I: Iterator<Item = B> + Clone,
B: Borrow<Item<'a>>,
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a note: DelayedFormat::{new, new_with_locale} are in an impl block without a generic offset parameter. Because they don't take an offset as argument it would fail during type inference otherwise.

new_with_offset is in an impl block with the new generic offset parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant