Repository navigation
Conversation
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):
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 ( print(new Date(1875619980085361).setUTCMinutes(-150119987617));
print(new Date(8.64e15 - 1).setUTCHours(-2501999793));
print(new Date(60000).setUTCSeconds(2 ** 53, -(2 ** 53) * 1000));
The Bun side waits, with no PR. Its test is on the branch |
|
Preview build of e33f556: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft on purpose: for WebKit/WebKit first, not for a merge into the fork. No user report.
Problem
setHours,setMinutes,setSecondsand 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
makeTime(), whichDate.UTCuses.main.JSTests/stress/date-time-setters-maketime-order.jsfails onmain. The test262 Date directories: same 1260 results.Background
mainhas the same file. [JSC] Make Date.UTC simpler WebKit/WebKit#2942 gaveDate.UTCmakeTime()and left the other callers open.Downsides
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.cpphas no fork change. It is blob683170f9beon this fork'smain(680ef47cec, which bun pins) and on WebKit/WebKitmain(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.Date.UTCand the Date constructormakeTime(). 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.Date.UTConly (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 forsetHours,setMinutes,setSecondsand their UTC forms. I cannot file in tc39/test262.ArrayStorage(cherry-pick WebKit/WebKit#75376) #785 were.WEBKIT_VERSIONbump and one test file,test/js/bun/jsc/date-time-setters-maketime-order.test.ts, on the branchrobobun/c6d77efb/date-time-setters-maketime-orderof oven-sh/bun. On Bun 1.4.3, 7 of its 8 tests fail. The other one is a control with ordinary arguments.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.
new Date(8.64e15 - 1).setUTCHours(-2501999793)was -367199337600000 and is -367199337600001.Date.UTCgive, which is not the exact sum.new Date(60000).setUTCSeconds(2 ** 53, -(2 ** 53) * 1000)was 60000 and is 60416, asDate.UTC(1970, 0, 1, 0, 1, 2 ** 53, -(2 ** 53) * 1000)is.setMillisecondsandsetUTCMillisecondsrun 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=nehalembuild and a ThinLTO build with the flags of the shippedltolane, clang 23.1.2)7ff8000000000000, as before.built-ins/Date,annexB/built-ins/Date,intl402/Date: 1260 runs, all pass on the parent and on this commit.run-jsc-stress-testsonstress/date-*: 715 runs, 0 fail.//@ runDefaultand 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_opencounters, user space, per call = (count(2N) - count(N)) / N, TZ=US/Pacific, parent680ef47cec. 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.)dateToDaysFrom1970()andmakeTime()are inlined wheregregorianDateTimeToMS()was called, and the replaced fields are no longer zeroed, converted and added.setMillisecondsorsetUTCMilliseconds(+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.setNewValueFromTimeArgs: 3078 -> 3403 bytes (ThinLTO), 1801 -> 1957 (non-LTO). jsc.text: +320 and +160 bytes.DateCache::gregorianDateTimeToMS: unchanged.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.Self-review. A review of the first version of this branch said: keep the code, change the venue. What changed:
robobun/c6d77efb/utc-time-setters-from-time-value. 0 of 2,400,000 results differ from this commit.JSTests/README.md://@ runDefault, its sweep bounded bytestLoopCount, 35,670 checks where it had 139,413.new Date(-NaN)storesfff8000000000000. The time setters store7ff8000000000000, before and after.680ef47cec, the commit bun pins now.Not in this PR
DateCache::gregorianDateTimeToMS()andWTF::timeToMS()stay. The date setters call the former, and so does bun, with int32 fields, for which both orders are exact.66ac271a67builds and passes both stress tests.valueOf.