-
Notifications
You must be signed in to change notification settings - Fork 534
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
Mdf
performance and docs
#1438
Mdf
performance and docs
#1438
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1438 +/- ##
=======================================
Coverage 91.85% 91.86%
=======================================
Files 40 40
Lines 17469 17471 +2
=======================================
+ Hits 16046 16049 +3
+ Misses 1423 1422 -1 ☔ View full report in Codecov by Sentry. |
I rebased the documentation commit to be earlier in the PR, that should make reviewing a bit easier. And there was one commit I had left after the |
It introduced a couple of extra checks in performance-sensitive code.
Nice, thank you! |
The
Mdf
types reason for existence is to have a way to convert between ordinal dates and month-day dates efficiently. It is a core part of the performance of chrono, we use it internally all over the place. Before converting it to returnResult
s in the 0.5 branch I need a benchmark to not materially regress its performance.Turns out I already regressed it by ca. 25% (at least according to the new benchmark) since chrono 0.4.24 😱.
After the benchmark in the first commit, the next three commits recover this lost performance by reverting needless checks:
Mdf::new
that are also covered by the table lookupMdf
exists for.internals
module part 4 #1432 tries to makeNaiveDate::from_mdf
share code withNaiveDate::from_ordinal_and_flags
. It looks clean, but that introduces a bunch of unneeded checks. I added a dedicated methodMdf::ordinal_and_flags
and madeNaiveDate::from_mdf
use that.NonZeroI32
insideNaiveDate
#1207 introduced a useless check against zero withNonZeroI32::new
.Then there are two commits that improve the performance a little bit more.
And while working on it I wrote some doc comments for the methods on
Mdf
.Benchmark results
Chrono 0.4.24
Before
After
I have the commits that extend this PR to convert an
Mdf
to returnResult
s ready, but would like to base them on this PR to the main branch.