time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.06k stars 261 forks source link

Rework to use `datealgo` library #634

Closed nakedible closed 8 months ago

nakedible commented 8 months ago

This pull does quite a big root-canal rework:

The results of this are performance gains:

Benchmark Time Change Comment
Date: from_calendar_date 7.4313 ns -25.644% Performance has improved
Date: from_ordinal_date 11.771 ns +31.051% Performance has regressed
Date: from_iso_week_date 15.810 ns -19.704% Performance has improved
Date: from_julian_day 9.2566 ns -38.340% Performance has improved
Date: year 2.1663 ns +2.6444% No change in performance detected.
Date: month 1.8139 ns -55.446% Performance has improved
Date: day 2.0914 ns -42.576% Performance has improved
Date: ordinal 4.1022 ns +149.47% Performance has regressed
Date: iso_week 10.260 ns +4.8933% No change in performance detected.
Date: sunday_based_week 7.7664 ns -2.8478% No change in performance detected.
Date: monday_based_week 7.4018 ns -10.981% Performance has improved
Date: to_calendar_date 1.8292 ns -61.345% Performance has improved
Date: to_ordinal_date 4.1561 ns +150.49% Performance has regressed
Date: to_iso_week_date 10.139 ns -1.0213% No change in performance detected.
Date: weekday 4.9401 ns -23.894% Performance has improved
Date: next_day 2.5907 ns -42.514% Performance has improved
Date: previous_day ns 2.0693 +3.5332% No change in performance detected.
Date: to_julian_day 3.0313 ns -31.273% Performance has improved
Date: midnight 1.7777 ns -28.381% Performance has improved
Date: with_time 1.9008 ns +4.5041% No change in performance detected.
Date: with_hms 7.3927 ns -2.9789% No change in performance detected.
Date: with_hms_milli 8.1539 ns +2.3146% No change in performance detected.
Date: with_hms_micro 10.025 ns +11.291% No change in performance detected.
Date: with_hms_nano 7.8717 ns -0.9628% No change in performance detected.
Date: add 9.2316 ns -39.057% Performance has improved
Date: add_std 8.0633 ns -47.409% Performance has improved
Date: add_assign 11.474 ns -33.455% Performance has improved
Date: add_assign_std 8.4556 ns -43.950% Performance has improved
Date: sub 9.1271 ns -36.140% Performance has improved
Date: sub_std 7.6889 ns -47.200% Performance has improved
Date: sub_assign 10.856 ns -38.463% Performance has improved
Date: sub_assign_std 11.638 ns -36.007% Performance has improved
Date: sub_self 7.4871 ns -17.402% Performance has improved
Date: partial_ord 326.20 ps -3.2359% No change in performance detected.
Date: ord 337.50 ps +3.7494% No change in performance detected.

As can be seen, the performance gains are quite sizeable. The only regressions are from_ordinal_date, ordinal and to_ordinal_date, all of which are obvious. Ordinal based operations are not terribly common, so I would expect the improvements in real world usage to be a clear net positive.

Disclaimer: I am the creator of the datealgo library, although I do not claim credit for the algorithms themselves.


NOTE: Pull is still a draft, so code has a bunch of FIXME comments and other things. Also, performance results are preliminary as benchmarks are noisy, so more careful benchmarking is still needed. However, I wanted to get this out early, in order to get feedback before completing the rest.

nakedible commented 8 months ago

@jhpratt We had a discussion elsewhere how time library optimization is important. This is probably quite relevant :-)

jhpratt commented 8 months ago

This is basically why the README says that it's best to float an idea by me first. To be direct, there is zero chance of me relying on another crate for the core functionality of time, particularly by an author that I am not familiar with. This is primarily due to supply chain security concerns.

With that said, I have no issue replacing the algorithms if they can be made more performant. The benchmarks in this crate are far from perfect in capturing the general use case, so they should be taken with a grain of salt. Of course, large improvements aren't much of a question.

One more note, I am not interested in changing Date to be represented as year-month-day instead of year-ordinal. The latter is used as it simplifies quite a bit.

nakedible commented 8 months ago

Don't worry – I knew I was taking a risk spending the time on this without talking to you first. But I also suspected that I'd have no chance at all to convince you if I didn't show significantly superior performance across the board, because as you don't know me it doesn't carry very far if I just say to you that "trust me, it's better this way". And it's good to get a clear answer as to what you think of it.

At least now you know that these performance gains are possible and you can look at the algorithms in datealgo: https://github.com/nakedible/datealgo-rs/blob/master/src/lib.rs

To be clear, I'm not personally interested in doing the work to duplicate the algorithms inside datealgo bit by bit in the time crate. The license is the same, so feel free to adapt whatever you want though – I'm fully supportive of that. I will however keep benchmarking against the time crate to see how the optimizations progress :-)

I'll close this pull as I don't intend to take it further, but I'll leave the repo and code around for reference if anyone else stumbles upon it.

jhpratt commented 8 months ago

Sounds good. Given the performance numbers, I will definite be looking into changing the implementations are some point. It's not a priority of mine, as I'm working on tzdb integration at the moment.