-
Notifications
You must be signed in to change notification settings - Fork 531
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
New Formatter
to replace DelayedFormat
#1163
Conversation
83f64e9
to
92e5477
Compare
11f6c3b
to
e2d998d
Compare
I added the ability to format with formatting parameters that can work without allocations in We need two adapters for This does add some complexity, but not too much in my opinion. And it makes formatting with |
27c41a5
to
4df403b
Compare
I really like this PR for the performance benefits. A request for your PRs @pitdicker , are you able to set your PRs to Draft until you feel done with additions? I know this won't be possible for all PRs as many related things can be in flux, but maybe where it is possible. While I don't mind doing PR reviews, it might be more efficient for both of us if I know that a PR is "still cooking" before deciding to dive in. Just a suggestion. Thanks for putting so much great work into this crate! 🙂 |
Thank for the reviews 😄. I intended this to be done. This PR and two others (maybe more are coming) are split of from my branch to add a new formatting API which I keep working at, sorry.... Now at 50 commits 😞 In this PR I adjusted the benchmarks to be comparable to the Before:
The interesting numbers are:
With my WIP branch that relies on this PR the numbers are:
The new benchmarks
But I also found a way to optimize Most of our time with formatting is spend converting numbers to strings of decimals. Printing the 9 digits of a nanosecond takes for example ca. 40% of the time spend in The trick is to copy number formatting for small numbers from |
Disclaimer: Of course a lot of the performance depends on which formatting items are used. The benchmarked RFC 3339 format only uses numbers for example. So these numbers are only an indication. |
Included a commit from my WIP branch here: collect all formatting tests in the |
5eca600
to
24b3fcb
Compare
1a27f6d
to
e22178b
Compare
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1163 +/- ##
==========================================
+ Coverage 91.24% 91.30% +0.05%
==========================================
Files 38 38
Lines 17062 17039 -23
==========================================
- Hits 15568 15557 -11
+ Misses 1494 1482 -12
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
e22178b
to
d18a8be
Compare
src/format/formatting.rs
Outdated
Formatter { date, time, offset, items, locale: default_locale() } | ||
} | ||
|
||
/// Makes a new `DelayedFormat` value out of local date and time, UTC offset and locale. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant:
/// Makes a new `Formatter` value out of ...
Off: Offset + Display, | ||
{ | ||
/// Makes a new `Formatter` value out of local date and time and UTC offset. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest mentioning
/// Uses the `default_locale()` as the locale.
9d59d11
to
5bdb5c4
Compare
5bdb5c4
to
d3df5ed
Compare
d3df5ed
to
cef3f54
Compare
Add a new formatter that can work without allocating and has less performance overhead, as mentioned in #1127 (comment).
I have made an effort to make the formatting code more readable, and split it up into reasonable commits.
My first PR to chrono that adds functionality but ends up with less lines 😄.I would like to depend on this PR to get #1035 over the finish line. Serialization worked without allocating before, and the new offset formatter should work without allocating in order to use it there.This doesn't yet include extra benchmarks or the rest of my proposal for the new formatting API. But I have enough in another branch to test it all works.
Four standalone
format*
functionschrono::format
are deprecated: they were near unusable, taking&mut Formatter<'_>
as an argument, which can't be constructed outside of aDisplay
implementation.Why a new formatter?
This adds a new type
Formatter<I, O>
, that can replaceDelayedFormat<I>
in a new formatting API.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 aString
. In case ofDateTime<FixedOffset>
andDateTime<Local>
this involves formatting the offset, as there is no name available. This is responsible for 20~25% of the ca. 40% overhead in theformat_with_items
benchmark.Formatter<I, O>
takes the offset as a generic, so it can delay formatting the timezone name until it is needed, which often is never.I made a couple of other changes that all reduce the overhead a tiny bit or help readablility: split the formatting function into smaller methods on
Formatter
, make better use ofmatch
, format using a smaller integer typei32
, and delay getting locale strings until use.We also format to
impl Write
instead ofString
like in #1126 to work withoutalloc
.All combined we have only about ~10% overhead left compared to the
format_manual
benchmark, which seems good enough to me.