Skip to content
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
e88e66e
Use bnum to define an InternalDuration struct
robot-head Jun 22, 2025
93f5315
Finish addressing all implications
robot-head Jun 22, 2025
ece5d8f
Merge branch 'main' into ms/optimize_duration
robot-head Jun 22, 2025
c546ae3
Remove TimeDuration completely
robot-head Jun 22, 2025
9c4b638
Merge branch 'ms/optimize_duration' of https://github.com/robot-head/…
robot-head Jun 22, 2025
22a2d41
Fix compile error
robot-head Jun 22, 2025
5cfb206
Fix lint
robot-head Jun 22, 2025
87c8513
Fix tests
robot-head Jun 22, 2025
21db4f5
Fix tests and lint errors
robot-head Jun 22, 2025
45d3262
Fix depcheck
robot-head Jun 22, 2025
a2e677a
cargo run -p diplomat-gen
robot-head Jun 22, 2025
9d46a14
Fix sign calculations
robot-head Jun 22, 2025
cbd388e
Fix compile error
robot-head Jun 22, 2025
0f67ff8
Fix duration sign handling and time diff
robot-head Jun 22, 2025
1a78f43
Merge pull request #6 from robot-head/codex/fix-failing-tests
robot-head Jun 22, 2025
f22a1d1
Fix duration sign handling
robot-head Jun 22, 2025
25ed765
Fix lint
robot-head Jun 22, 2025
6b9b646
Merge pull request #7 from robot-head/6o0p22-codex/fix-failing-tests
robot-head Jun 22, 2025
7123b03
Fix lint
robot-head Jun 22, 2025
79cdb11
Fix duration calc
robot-head Jun 22, 2025
9fd53fe
Respond to comments
robot-head Jun 23, 2025
7aa9d0f
Merge remote-tracking branch 'origin/main' into ms/optimize_duration
robot-head Jul 4, 2025
c0eecb0
Post-merge fixes
robot-head Jul 4, 2025
74442e4
Fix sign on NormalizedDurationRecord
robot-head Jul 5, 2025
12abb62
Fix nudge_to_day_or_time
robot-head Jul 5, 2025
06e9f9d
Add from_components method to NormalizedTimeDuration for creating ins…
Jul 31, 2025
ee85fa0
Refactor U48 type definition to use BUintD16
Jul 31, 2025
ef1f029
Refactor Duration constructor to use absolute values for time components
Jul 31, 2025
4ee26b4
Merge branch 'main' into ms/optimize_duration
nekevss Aug 3, 2025
0e72c84
Fix broken tests + update various abstract operations
nekevss Aug 3, 2025
3d14622
Update the FFI
nekevss Aug 3, 2025
7895f65
Update tzif-inspect + add test case to from_partial_duration
nekevss Aug 3, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


[features]
default = ["sys"]
Expand Down
1 change: 1 addition & 0 deletions depcheck/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ pub const BASIC_RUNTIME_DEPS: &[&str] = &[
"zerotrie",
"zerovec",
// Other deps
"bnum",
"libm",
"num-traits",
"stable_deref_trait",
Expand Down
31 changes: 28 additions & 3 deletions src/builtins/compiled/duration/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
OffsetDisambiguation, RelativeTo, RoundingIncrement, RoundingMode, RoundingOptions, Unit,
},
partial::PartialDuration,
Calendar, DateDuration, PlainDate, TimeDuration, TimeZone, ZonedDateTime,
Calendar, DateDuration, PlainDate, TimeZone, ZonedDateTime,
};

use core::{num::NonZeroU32, str::FromStr};
Expand Down Expand Up @@ -373,7 +373,20 @@ fn basic_negative_expand_rounding() {

#[test]
fn rounding_to_fractional_day_tests() {
let twenty_five_hours = Duration::from(TimeDuration::new(25, 0, 0, 0, 0, 0).unwrap());
let twenty_five_hours = Duration::new_unchecked(
Default::default(),
0,
0,
0,
0u8.into(),
25u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
);

let options = RoundingOptions {
largest_unit: Some(Unit::Day),
smallest_unit: None,
Expand Down Expand Up @@ -485,7 +498,19 @@ fn basic_subtract_duration() {
// days-24-hours-relative-to-zoned-date-time.js
#[test]
fn round_relative_to_zoned_datetime() {
let duration = Duration::from(TimeDuration::new(25, 0, 0, 0, 0, 0).unwrap());
let duration = Duration::new_unchecked(
Default::default(),
0,
0,
0,
0u8.into(),
25u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
0u8.into(),
);
let zdt = ZonedDateTime::try_new(
1_000_000_000_000_000_000,
Calendar::default(),
Expand Down
9 changes: 3 additions & 6 deletions src/builtins/core/calendar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,11 @@
//! Temporal compatible calendar implementations.

use crate::{
builtins::core::{
duration::{DateDuration, TimeDuration},
Duration, PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth,
},
builtins::core::{Duration, PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth},
iso::IsoDate,
options::{ArithmeticOverflow, Unit},
parsers::parse_allowed_calendar_formats,
TemporalError, TemporalResult,
DateDuration, TemporalError, TemporalResult,
};
use alloc::string::ToString;
use core::str::FromStr;
Expand Down Expand Up @@ -300,7 +297,7 @@ impl Calendar {
// duration.[[Milliseconds]], duration.[[Microseconds]], duration.[[Nanoseconds]]).
// 9. Let balanceResult be BalanceTimeDuration(norm, "day").
let (balance_days, _) =
TimeDuration::from_normalized(duration.time().to_normalized(), Unit::Day)?;
Duration::from_normalized_time(duration.to_normalized(), Unit::Day)?;

// 10. Let result be ? AddISODate(date.[[ISOYear]], date.[[ISOMonth]], date.[[ISODay]], duration.[[Years]],
// duration.[[Months]], duration.[[Weeks]], duration.[[Days]] + balanceResult.[[Days]], overflow).
Expand Down
9 changes: 3 additions & 6 deletions src/builtins/core/date.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ use icu_calendar::AnyCalendarKind;
use writeable::Writeable;

use super::{
calendar::month_to_month_code,
duration::{normalized::NormalizedDurationRecord, TimeDuration},
calendar::month_to_month_code, duration::normalized::NormalizedDurationRecord,
PartialYearMonth, PlainMonthDay, PlainYearMonth,
};
use tinystr::TinyAsciiStr;
Expand Down Expand Up @@ -206,9 +205,7 @@ impl PlainDate {
// "day").[[Days]].
let days = duration
.days()
.checked_add(
TimeDuration::from_normalized(duration.time().to_normalized(), Unit::Day)?.0,
)
.checked_add(Duration::from_normalized_time(duration.to_normalized(), Unit::Day)?.0)
.ok_or(TemporalError::range())?;

// 7. Let result be ? AddISODate(plainDate.[[ISOYear]], plainDate.[[ISOMonth]], plainDate.[[ISODay]], 0, 0, 0, days, overflow).
Expand Down Expand Up @@ -288,7 +285,7 @@ impl PlainDate {
let result = self.internal_diff_date(other, resolved.largest_unit)?;

// 10. Let duration be ! CreateNormalizedDurationRecord(result.[[Years]], result.[[Months]], result.[[Weeks]], result.[[Days]], ZeroTimeDuration()).
let mut duration = NormalizedDurationRecord::from_date_duration(*result.date())?;
let mut duration = NormalizedDurationRecord::from_date_duration(result.date())?;
// 11. If settings.[[SmallestUnit]] is "day" and settings.[[RoundingIncrement]] = 1, let roundingGranularityIsNoop be true; else let roundingGranularityIsNoop be false.
let rounding_granularity_is_noop =
resolved.smallest_unit == Unit::Day && resolved.increment.get() == 1;
Expand Down
11 changes: 7 additions & 4 deletions src/builtins/core/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,16 @@ impl PlainDateTime {
// 4. Let calendarRec be ? CreateCalendarMethodsRecord(dateTime.[[Calendar]], « date-add »).

// 5. Let norm be NormalizeTimeDuration(sign × duration.[[Hours]], sign × duration.[[Minutes]], sign × duration.[[Seconds]], sign × duration.[[Milliseconds]], sign × duration.[[Microseconds]], sign × duration.[[Nanoseconds]]).
let norm = NormalizedTimeDuration::from_time_duration(duration.time());
let norm = NormalizedTimeDuration::from_duration(duration);

// TODO: validate Constrain is default with all the recent changes.
// 6. Let result be ? AddDateTime(dateTime.[[ISOYear]], dateTime.[[ISOMonth]], dateTime.[[ISODay]], dateTime.[[ISOHour]], dateTime.[[ISOMinute]], dateTime.[[ISOSecond]], dateTime.[[ISOMillisecond]], dateTime.[[ISOMicrosecond]], dateTime.[[ISONanosecond]], calendarRec, sign × duration.[[Years]], sign × duration.[[Months]], sign × duration.[[Weeks]], sign × duration.[[Days]], norm, options).
let result =
self.iso
.add_date_duration(self.calendar().clone(), duration.date(), norm, overflow)?;
let result = self.iso.add_date_duration(
self.calendar().clone(),
&duration.date(),
norm,
overflow,
)?;

// 7. Assert: IsValidISODate(result.[[Year]], result.[[Month]], result.[[Day]]) is true.
// 8. Assert: IsValidTime(result.[[Hour]], result.[[Minute]], result.[[Second]], result.[[Millisecond]],
Expand Down
Loading
Loading