New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] Fix startOf/endOf DST issues while boosting performance #4338
Conversation
This looks great. Are these benchmarks from Moment's benchmarks suite or separate? |
From moment's benchmark suite - by running |
src/lib/create/date-from-array.js
Outdated
var date = new Date(y, m, d, h, M, s, ms); | ||
if (arguments.length === 3) { | ||
// Special-case: handles dates that don't start at midnight | ||
date = new Date(remapYears ? y + 400 : y, m, d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this behaves differently depending on whether those lower-order units are undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it did, but it doesn't. I've removed it now.
(the Brazil case is still fixed):
TZ='America/Sao_Paulo' node -e "$(cat << EOF
var moment = require('moment');
// Bug: Brazil was showing wrong end-of-day for 2016-10-16:
console.log("Expected: 2016-10-16 23:59:59");
console.log("Actual: " + moment("2016-10-16").endOf('day').format("YYYY-MM-DD HH:mm:ss"));
// Check start of day:
console.log("Expected: 2016-10-16 01:00:00");
console.log("Actual: " + moment("2016-10-16T12:34").startOf('day').format("YYYY-MM-DD HH:mm:ss"));
EOF
)"
@ichernev this is a good one for you to look at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good. However -- there is a problem with DST at the start of units (mostly days and even months).
When doing such an extensive rewrite of start/endOf it makes sense to also take that long-standing issue into account.
src/lib/moment/start-end-of.js
Outdated
/* falls through */ | ||
time = this._d.valueOf(); | ||
this._d.setTime(time - mod(time, MS_PER_MINUTE)); | ||
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just do time = this._d.valueOf(); time -= mod(time, MS_PER_MINUTE); break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a (premature) optimisation: there's no point calling updateOffset
hooks when you're only moving to start/end of minutes or seconds.
src/lib/moment/start-end-of.js
Outdated
if (units === 'quarter') { | ||
this.month(Math.floor(this.month() / 3) * 3); | ||
time = this._d.valueOf(); | ||
this._d.setTime(time - mod(time, MS_PER_SECOND)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
In general most of the switch uses break, and only the last 2 use return, which is confusing. I'm sure the perf will be fine.
src/lib/moment/start-end-of.js
Outdated
var MS_PER_SECOND = 1000; | ||
var MS_PER_MINUTE = 60 * MS_PER_SECOND; | ||
var MS_PER_HOUR = 60 * MS_PER_MINUTE; | ||
var MS_PER_24_HOUR_DAY = 24 * MS_PER_HOUR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use MS_PER_DAY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a deliberate name choice. DST means 'MS_PER_DAY' would be a misnomer.
src/lib/moment/start-end-of.js
Outdated
case 'minute': | ||
time = this._d.valueOf(); | ||
this._d.setTime(time - mod(time, MS_PER_MINUTE) + MS_PER_MINUTE - 1); | ||
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here and the one after
@ichernev Updated as requested. Do you have a specific issue in mind for the DST changes you think these changes fail to address? |
This PR indeed seems to fix issue DST startOf / endOf. Can this be merged? |
@ichernev - any thoughts? |
First off, this is awesome. There are a lot of tickets you are referencing. I would like to see tests with the specific cases that are mentioned in the issues (and their comments) showing that this PR does, in fact, solve the issue. Then, we can ensure that each particular issue is fixed. You could even comment before the test about which Github issue you are testing. Additional generalized tests are ok, if there is a consistent theme. I don't mind if you don't fix other bugs "while you're at it" in this one swoop - I try to avoid that behavior for those reading, I find this guide informative. The above tests should prevent regressions when we fix future bugs. Thanks a lot @ashsearle for your help in this codebase :-) |
src/lib/moment/start-end-of.js
Outdated
var MS_PER_SECOND = 1000; | ||
var MS_PER_MINUTE = 60 * MS_PER_SECOND; | ||
var MS_PER_HOUR = 60 * MS_PER_MINUTE; | ||
var MS_PER_DAY = 24 * MS_PER_HOUR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind renaming this back to 24_HR_DAY or what you had before, especially if you leave a comment that some days during DST do not have 24 hours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the MS_PER_DAY variable's only used once, I've inlined to avoid any further debate :-)
src/test/moment/start_end_of.js
Outdated
|
||
test('startOf for year zero', function (assert) { | ||
var m = moment('0000-02-29T12:34:56.789Z').parseZone(); | ||
assert.equal(m.clone().startOf('ms').toISOString(), '0000-02-29T12:34:56.789Z', 'startOf millisecond should preserver year'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean "preserve"?
(preserver -> preserve throughout these tests.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I sure did. I've corrected the spelling now.
assert.equal(m.clone().endOf('M').toISOString(), '0000-02-29T23:59:59.999Z', 'endOf month should preserver year'); | ||
assert.equal(m.clone().endOf('Q').toISOString(), '0000-03-31T23:59:59.999Z', 'endOf quarter should preserver year'); | ||
assert.equal(m.clone().endOf('y').toISOString(), '0000-12-31T23:59:59.999Z', 'endOf year should preserver year'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some more tests about the issues you are referencing.
For example, I'm not convinced this PR will solve #1990
There are examples in #2749, #3580, and in #3132, #4152.
We should be able to construct test in particular locales. I'm not sure about testing in conjunction with moment-tz, but let me know how far you get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to add tests that'll run on Travis for some of these things...
(Issue #952 addresses the difficulty of this and talks about a brute-force approach running tests across all timezones using a grunt zones
task - but that task doesn't exist...)
When I was working on tests, I'd try and identify a specific date and timezone showing the problem, and run eye-ball a test run from a bash script. Something like this:
#!/bin/bash
moment=./build/umd/moment.js
echo "Issue 2749:"
TZ='Europe/Copenhagen' node -e "$(cat << EOF
var moment = require('${moment}');
console.log("Expected:", moment('2015-10-25T02:00:00.000+01:00').unix());
console.log("Actual: ", moment('2015-10-25T02:00:00.000+01:00').startOf('hour').unix());
EOF
)"
echo "Issue 3132:"
TZ='America/Sao_Paulo' node -e "$(cat << EOF
var moment = require('${moment}');
// Bug: Brazil was showing wrong end-of-day for 2016-10-16:
console.log("Expected: 2016-10-16 23:59:59");
console.log("Actual: " + moment("2016-10-16").endOf('day').format("YYYY-MM-DD HH:mm:ss"));
// Check start of day:
console.log("Expected: 2016-10-16 01:00:00");
console.log("Actual: " + moment("2016-10-16T12:34").startOf('day').format("YYYY-MM-DD HH:mm:ss"));
EOF
)"
Talking about #1990 specifically though: that one is fixed because the problem is in browser variation in how setMinutes/Seconds/Milliseconds is implemented (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1084547) - by avoiding these methods, we avoid the bug.
1 similar comment
Bumping this PR to see if it can make it into an upcoming release. The DST issue caused non-trivial problems for some of our users in Brazil this week when DST rolled over on Sunday. Difficult for us to work around this in our own code. Would like to avoid getting snagged by this the next time DST rolls over there, if possible... |
Sorry for the delay! This will come in next release (not tonight though!) |
Thanks @ashsearle - just a couple quick things and good to go! |
@marwahaha I've rebased this branch (to bring it up to date), and addressed your latest review comments. |
These changes to
startOf
andendOf
fix several DST-related issues:Date.UTC
/new Date
for units bigger than hours - fixes endOf('day') fails on days that don't start at midnight #3132, fixes Stepping through days is not correct for Asia/Beirut. #4152Additionally, the changes supersede these pull requests: #3620, #4164 and #4254
Benchmark results
Additionally, fixed one more bug:
moment('0000-02-29')
was created in year 1900 then reverted to year 0. Unfortunately Feb 29th doesn't exist in 1900, so the moment ended up as0000-03-01
instead.New tests have been added to
start_end_of.js
that would have failed in earlier releases.