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

Rework to use datealgo library #634

Closed
wants to merge 1 commit into from

Conversation

nakedible
Copy link

This pull does quite a big root-canal rework:

  • Date internal representation is changed from (year: 21 bits, ordinal: 9 bits) to (year: 21 bits, month: 4 bits, day: 5 bits)
  • All internal date manipulation algorithms replaced by calling into datealgo library

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
Copy link
Author

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

@jhpratt
Copy link
Member

jhpratt commented Oct 31, 2023

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
Copy link
Author

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.

@nakedible nakedible closed this Oct 31, 2023
@jhpratt
Copy link
Member

jhpratt commented Nov 1, 2023

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.

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.

None yet

2 participants