Skip to content

[JSC] Date time setters should add their fields in the order of MakeTime - #896

Draft
robobun wants to merge 1 commit into
mainfrom
robobun/c6d77efb/date-time-setters-maketime
Draft

robobun wants to merge 1 commit into
mainfrom
robobun/c6d77efb/date-time-setters-maketime

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Draft on purpose: for WebKit/WebKit first, not for a merge into the fork. No user report.

Problem

  • setHours, setMinutes, setSeconds and their UTC forms do not add in the order of MakeTime. Beyond 2^53 ms the result differs: new Date(1875619980085361).setUTCMinutes(-150119987617) is -7131579276994640. MakeTime, V8 and the exact sum give -7131579276994639.
  • fillStructuresUsingTimeArgs() (runtime/DatePrototype.cpp) sums the arguments into one double. gregorianDateTimeToMS() adds the kept fields after.

Fix

  • The four fields stay separate doubles and go to makeTime(), which Date.UTC uses.
  • Correct because the result is MakeTime's formula in doubles: 0 of 450,000 huge-argument calls differ from V8. 0 of 4,200,000 ordinary calls differ from main.
  • Verified: new JSTests/stress/date-time-setters-maketime-order.js fails on main. The test262 Date directories: same 1260 results.
  • Self-reviewed: 34 concerns raised, 32 addressed (Notes).

Background

Downsides

  • Results change only beyond 2^53 ms: 1 ms for one argument, more when arguments of 2^58 ms or more cancel. The result then follows MakeTime, not the exact sum.
  • x64 ThinLTO: setMilliseconds(NaN) runs 2 to 5 more instructions. A call that returns a time value runs 35 to 55 fewer. The function grows by 325 bytes.
Notes

Route. This PR does not ask for a merge into the fork as it stands.

  • DatePrototype.cpp has no fork change. It is blob 683170f9be on this fork's main (680ef47cec, which bun pins) and on WebKit/WebKit main (ec62b60d6c, 2026-10-24). So the patch applies upstream as it is, and the test is a new file. A merge here first would be the fork's first change in that file, carried through each upstream sync.
  • @sosukesuzuki: I cannot file the bugs.webkit.org entry or the WebKit/WebKit PR. [JSC] Make Date.UTC simpler WebKit/WebKit#2942 (253044@main, bug 243446) gave Date.UTC and the Date constructor makeTime(). Its review asked "Aren't these operations used elsewhere in the Date API?", and the answer left the other callers open. This patch is that answer for the time setters.
  • I could not search Bugzilla for an existing report: bugs.webkit.org does not resolve from the machine this ran on. A search of the open PRs of WebKit/WebKit and of this repository finds none for it.
  • test262 pins the order of MakeTime for Date.UTC only (built-ins/Date/UTC/fp-evaluation-order.js). The fixed rows at the top of the stress test fail on JSC and pass on V8. They can become the same file for setHours, setMinutes, setSeconds and their UTC forms. I cannot file in tc39/test262.
  • What I expect to happen to this PR: it closes when a sync brings the upstream commit, or it becomes a cherry-pick of that commit, as [JSC] IRO's ArithAbs transform can pin freed range bound proofs (cherry-pick WebKit/WebKit#72679) #530 and [JSC] Array profiles should not turn ordinary arrays into ArrayStorage (cherry-pick WebKit/WebKit#75376) #785 were.
  • The question for a maintainer: does the fork take this before upstream does? If yes, this commit can merge as it is.
  • The Bun side waits for the answer, with no PR. It is a WEBKIT_VERSION bump and one test file, test/js/bun/jsc/date-time-setters-maketime-order.test.ts, on the branch robobun/c6d77efb/date-time-setters-maketime-order of oven-sh/bun. On Bun 1.4.3, 7 of its 8 tests fail. The other one is a control with ordinary arguments.
  • That test on a Bun with this change: a local debug build of that branch (oven-sh/bun 179664ea89). With the pinned WebKit, 7 of 8 fail. With --webkit-version=autobuild-preview-pr-896-e33f5568, the preview build of this PR, 8 of 8 pass.

Who is hit. Nobody that I know of: 0 issues, and 0 test262 or JSTests files see it. A result moves only in two cases.

  • One argument worth between 2^53 and 2^53 + 86,400,000 ms (285,000 years): the result moves by 1 ms, to the exact sum. new Date(8.64e15 - 1).setUTCHours(-2501999793) was -367199337600000 and is -367199337600001.
  • Several arguments of about 2^58 ms (9 million years) or more that cancel: the result moves by 32 ms and up, to what MakeTime and Date.UTC give, which is not the exact sum. new Date(60000).setUTCSeconds(2 ** 53, -(2 ** 53) * 1000) was 60000 and is 60416, as Date.UTC(1970, 0, 1, 0, 1, 2 ** 53, -(2 ** 53) * 1000) is.
  • setMilliseconds and setUTCMilliseconds run the same code and never differed. The date setters have the same mechanism for their day argument: that is [JSC] Give the date setters the MakeDay of Date.UTC, with an exact int64 year/month split #616.

Results (Release -march=nehalem build and a ThinLTO build with the flags of the shipped lto lane, clang 23.1.2)

  • ordinary arguments: 0 of 4,200,000 results differ from the parent, in 7 time zones, on Dates nothing has read and on Dates a getter has read.
  • huge and special arguments, TZ=UTC: 0 of 450,000 results differ from node 26. The parent differs on 11,211. The 224,597 calls of a UTC form were also compared with MakeTime's formula evaluated in doubles (the model at the top of the stress test): 0 differ. The parent differs on 5,578.
  • the bits of a NaN result, in a matrix of 253,130 calls with special arguments: 252,111 calls return NaN, and each returns and stores 7ff8000000000000, as before.
  • test262 built-ins/Date, annexB/built-ins/Date, intl402/Date: 1260 runs, all pass on the parent and on this commit. run-jsc-stress-tests on stress/date-*: 715 runs, 0 fail.
  • the stress test has //@ runDefault and runs in 68 ms. 30 of 30 single-edit mutants of the change fail it (each field a setter keeps or stores, the arity checks, the day, local against UTC, the order of the sum, the non-finite exit, TimeClip, the exception check). The 3 mutants that only change the order of the sum pass every section but the one with huge arguments.

Cost per call (x64 Linux, perf_event_open counters, user space, per call = (count(2N) - count(N)) / N, TZ=US/Pacific, parent 680ef47cec. 41 call orders with ordinary arguments: each setter alone, after a getter, before a getter, on 1024 Dates, on a new Date, four setters in a row, the shapes of dayjs and date-fns.)

  • ordinary arguments, ThinLTO: 34.7 to 55.2 fewer instructions and 1.6 to 3.2 fewer branches per call, 0 of 41 orders above the parent. dateToDaysFrom1970() and makeTime() are inlined where gregorianDateTimeToMS() was called, and the replaced fields are no longer zeroed, converted and added.
  • same, non-LTO: 21.9 to 41.8 fewer instructions, -0.7 to +0.2 branches, 0 of 41 above.
  • a non-finite argument on a valid Date, 45 call orders: -16.7 to +5.0 instructions under ThinLTO. 10 orders are above the parent, all setMilliseconds or setUTCMilliseconds (+1.9 to +5.0): the kept fields are converted before the sum is known not to be finite. Without LTO: -10.1 to +6.5, 28 above.
  • cycles per call, 23 call orders, 15 interleaved A/B runs, with the A/A floor of the same runs in parentheses. ThinLTO, JIT on: -8.7% to -5.3% (-2.0% to +0.6%). ThinLTO, JIT off: -8.1% to -3.0% (-1.1% to +0.7%). Non-LTO, JIT on: -9.0% to -3.8% (-0.5% to +0.3%).
  • setNewValueFromTimeArgs: 3078 -> 3403 bytes (ThinLTO), 1801 -> 1957 (non-LTO). jsc .text: +320 and +160 bytes. DateCache::gregorianDateTimeToMS: unchanged.
  • arm64, not a native run, and measured on the previous base 79618395ee (the same Date files): the front-end IR of the real translation units retargeted to aarch64 with the lanes' CPU flags, and each host call counted under qemu-user. 9 to 41 fewer instructions per call on 66 of 66 ordinary cases in 5 configurations. Non-finite probes: +5 to +22.
  • Windows x64 was not measured.

Self-review. A review of the first version of this branch said: keep the code, change the venue. What changed:

  • The PR is a draft for the upstream route, with no bun pin PR.
  • A second commit that took the day of a UTC setter from the time value is gone from this branch. It is a performance change of its own: a UTC setter runs 28 to 206 fewer instructions per call than with this commit, a local setter -11 to +1 (x64, non-LTO). It needs benchmarks and the three UTC date setters before it is proposed anywhere.
  • That commit is kept on top of this one, with no PR, on the branch robobun/c6d77efb/utc-time-setters-from-time-value. 0 of 2,400,000 results differ from this commit.
  • The stress test follows JSTests/README.md: //@ runDefault, its sweep bounded by testLoopCount, 35,670 checks where it had 139,413.
  • The comment and the test no longer say that a Date holds one NaN. new Date(-NaN) stores fff8000000000000. The time setters store 7ff8000000000000, before and after.
  • The branch sits on 680ef47cec, the commit bun pins now.
  • Not done: the Bugzilla search and the test262 PR (see Route).

Not in this PR

Date.prototype.setHours, setMinutes, setSeconds and their UTC forms add their
time fields in another order than MakeTime. When a partial sum is beyond
2^53 ms, the result differs:

    new Date(1875619980085361).setUTCMinutes(-150119987617)
    // -7131579276994640. MakeTime gives -7131579276994639, the exact sum.
    new Date(60000).setUTCSeconds(2 ** 53, -(2 ** 53) * 1000)
    // 60000. MakeTime and Date.UTC(1970, 0, 1, 0, 1, 2 ** 53, -(2 ** 53) * 1000) give 60416.

MakeTime returns ((hour * msPerHour + min * msPerMinute) + sec * msPerSecond) + ms
and notes that the arithmetic is floating-point, is not associative, and has
to be done in that order. 253044@main made Date.UTC and the Date constructor
call makeTime(). The time setters kept their own sum:
fillStructuresUsingTimeArgs() added the arguments into one double that
started from the milliseconds of the Date, and
DateCache::gregorianDateTimeToMS() then added the fields the Date kept. So
the kept and the given fields met in two groups that were rounded apart.

fillStructuresUsingTimeArgs() now only stores each argument.
setNewValueFromTimeArgs() holds the four fields as doubles, each the argument
or the field of the Date, and passes them to makeTime(). The day is
dateToDaysFrom1970() of the year, month and day the Date has, and makeDate()
and DateCache::localTimeToMS() finish the value as they do for the
constructor. The fields of the Date come from where they came before, at the
same moment, so no time zone offset look-up is added. A time that is not
finite still ends in PNaN before the day is computed.

No result changes while every partial sum is below 2^53: each sum is then
exact in any order. One argument alone changes the result by 1 ms, and only
when it is worth between 2^53 and 2^53 + 86400000 ms. Several arguments that
cancel change it by more, from terms of about 2^58 ms.

* JSTests/stress/date-time-setters-maketime-order.js: Added.
* Source/JavaScriptCore/runtime/DatePrototype.cpp:
(JSC::fillStructuresUsingTimeArgs):
(JSC::setNewValueFromTimeArgs):
@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: draft on purpose. This is an upstream candidate, not a request to merge into the fork (see Route in the notes of the description).

How to reproduce (jsc shell of main 680ef47. In Bun and Node.js use console.log):

print(new Date(1875619980085361).setUTCMinutes(-150119987617));
print(new Date(8.64e15 - 1).setUTCHours(-2501999793));
print(new Date(60000).setUTCSeconds(2 ** 53, -(2 ** 53) * 1000));
  • main and Bun 1.4.3 print -7131579276994640, -367199337600000 and 60000.
  • This branch and Node.js v26.3.0 print -7131579276994639, -367199337600001 and 60416.
  • JSTests/stress/date-time-setters-maketime-order.js exits with 3 on main and with 0 on this branch, in 9 time zones.

The Bun side waits, with no PR. Its test is on the branch robobun/c6d77efb/date-time-setters-maketime-order of oven-sh/bun. On Bun 1.4.3 and on a local debug build of Bun with the pinned WebKit, 7 of its 8 tests fail. The other one is a control with ordinary arguments. On the same debug build with the preview build of this PR (--webkit-version=autobuild-preview-pr-896-e33f5568), 8 of 8 pass.

@github-actions

Copy link
Copy Markdown

Preview build of e33f556: autobuild-preview-pr-896-e33f5568

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.

1 participant