Migrate Duration to smallest numeric types possible for efficiency #366
Conversation
…tances from signed integer components
Fix two unit tests try_from to use cast_from
Many broken tests were related to DateDuration needing to be an intermediate computational version. For instance, days can exceed the normal threshold in some calculations. There was also a number of operations that needed to be updated the the current specification in order to fix the bugs that came from this current update. Amongst them were nudge_to_calendar_unit, nudge_to_day_or_time, various InternalDuration record operations. It's worth noting that this current update preserves the old "normalized" specification language. Those should probably be updated in follow ups to the newer language for specification consistency, but that seemed to be out of scope for the current PR.
| @@ -82,6 +82,7 @@ timezone_provider = { workspace = true, optional = true } | |||
| # System time feature | |||
| web-time = { workspace = true, optional = true } | |||
| iana-time-zone = { workspace = true, optional = true } | |||
| bnum = "0.13.0" | |||
There was a problem hiding this comment.
issue: hmm, I know this was @nekevss's suggestion, but I'm not really excited about adding a new dependency just for an optimization, especially one we haven't measured the benefit of)
suggestion: Can we implement one of the earlier suggestions to use narrow stdlib integer types and flatten Duration, and then have a separate discussion for adding the bnum dependency? I imagine that switchover will be cleaner.
@nekevss opinions?
(Given that this is reviewed already, I wouldn't recommend changing the PR until we decide what we want here, it's possible temporal_rs decides to take on the bnum dep)
There was a problem hiding this comment.
Honestly, the one thing I've been most iffy about is adding the extra dependency (to the point of debating looking up how to manually implement arbitrarily sized integers 😅)
Hmmmmmm, I can flatten the struct and do some of the specification updates in a separate PR. If I realized that was going to be such a big portion of this PR, then I would've wanted that split off in a different PR anyways. My general concern is that the resulting Duration type is going to be huge.
There was a problem hiding this comment.
For a stack type, huge isn't really a big deal most of the time.
For JS implementors it will likely be heap allocated. But it's not like people are making thousands of these, I think.
| @@ -56,42 +52,6 @@ pub mod ffi { | |||
| } | |||
| } | |||
|
|
|||
| impl TimeDuration { | |||
There was a problem hiding this comment.
note: TimeDuration is not used in V8 right now, removing this API will not need complex migration
| duration: &TimeDuration, | ||
| ) -> Result<Box<Self>, TemporalError> { | ||
| // TODO: deprecate? | ||
| pub fn add_time_duration(&self, duration: &Duration) -> Result<Box<Self>, TemporalError> { |
There was a problem hiding this comment.
observation: this is fine to remove
This PR makes a large portion of updates from #366 without bringing in the `bnum` crate. The changes are primarily as follows: - Updates Duration to flat unsigned fields - Removes the previous TimeDuration - NormalizedDurationRecord -> InternalDurationRecord - NormalizedTimeDuration -> TimeDuration There are a handful of API changes linked to this change that are primarily related to moving addition variations that were using the old TimeDuration.
Addresses #189 by implementing the latest suggestion here: #189 (comment)