time-rs / time

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

Remove unnecessary floating point remainder #526

Closed CryZe closed 1 year ago

CryZe commented 1 year ago

Because the code handles all the edge cases already, we can simply cast the integer seconds back to a floating point number to truncate it, instead of using seconds % 1.0 which is not only slower, but also bloats the binary, especially on no_std, as it brings in an implementation of fmod / fmodf.

codecov[bot] commented 1 year ago

Codecov Report

Merging #526 (ba68307) into main (01ca01a) will increase coverage by 0.0%. The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #526   +/-   ##
=====================================
  Coverage   98.4%   98.4%           
=====================================
  Files         76      76           
  Lines       8215    8221    +6     
=====================================
+ Hits        8082    8088    +6     
  Misses       133     133           
Impacted Files Coverage Δ
time/src/duration.rs 100.0% <100.0%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jhpratt commented 1 year ago

Neat optimization. I've confirmed that it reduces the amount of generated assembly, so it's definitely a win.

CryZe commented 1 year ago

Another thing to consider might be to port the algorithm from std, which nowadays basically abuses the fact that floating point numbers are really just the mantissa that represents "two integers" separated by a decimal point (through the exponent). So you could just extract those two integers without any loss of precision (as would be the case when trying to represent the fraction as its own floating point number). Though that's of course slower again.

jhpratt commented 1 year ago

Ah right, I forgot the algorithm changed a couple releases ago. Thanks for the reminder!

jhpratt commented 1 year ago

So…I just attempted to adapt that algorithm. It's a bit hacky, as I basically just forced the sign to be what it was supposed to be at the end, and still doesn't differentiate overflow and NaN (for the panic message). But in the end I am measuring a regression. That may be because of the exact benchmarks I am using, but it doesn't seem to be a super obvious gain. As such I'm going to keep your improvement and stop there.