Skip to content
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

Merged
merged 12 commits into from Jan 19, 2019

Conversation

ashsearle
Copy link
Contributor

@ashsearle ashsearle commented Dec 1, 2017

These changes to startOf and endOf fix several DST-related issues:

Additionally, the changes supersede these pull requests: #3620, #4164 and #4254

Benchmark results

Benchmark moment 2.19.3 this PR improvement
startOf second 2,598,171 ops/sec ±1.24% 5,322,216 ops/sec ±1.56% 2.0x
startOf minute 1,606,046 ops/sec ±0.98% 7,397,801 ops/sec ±0.92% 4.6x
startOf hour 1,069,176 ops/sec ±0.96% 5,015,984 ops/sec ±0.91% 4.6x
startOf date 811,815 ops/sec ±1.08% 1,162,031 ops/sec ±0.89% 1.4x
startOf day 827,911 ops/sec ±0.83% 1,170,806 ops/sec ±1.25% 1.4x
startOf isoWeek 274,302 ops/sec ±1.06% 973,431 ops/sec ±1.09% 3.5x
startOf week 298,240 ops/sec ±1.00% 1,188,010 ops/sec ±0.94% 3.9x
startOf month 653,227 ops/sec ±0.85% 1,447,224 ops/sec ±0.76% 2.2x
startOf quarter 381,496 ops/sec ±0.85% 1,221,562 ops/sec ±0.93% 3.2x
startOf year 425,796 ops/sec ±0.98% 2,047,534 ops/sec ±1.02% 4.8x
endOf second 200,544 ops/sec ±0.92% 5,496,719 ops/sec ±1.00% 27.4x
endOf minute 190,097 ops/sec ±1.02% 6,600,219 ops/sec ±0.94% 34.7x
endOf hour 331,129 ops/sec ±0.86% 4,612,042 ops/sec ±0.88% 13.9x
endOf date 249,577 ops/sec ±1.10% 1,239,091 ops/sec ±0.96% 4.9x
endOf day 251,496 ops/sec ±1.01% 1,240,042 ops/sec ±1.35% 4.9x
endOf isoWeek 160,238 ops/sec ±1.73% 917,050 ops/sec ±0.85% 5.7x
endOf week 174,334 ops/sec ±2.02% 1,216,138 ops/sec ±0.88% 6.9x
endOf month 200,123 ops/sec ±1.24% 1,480,412 ops/sec ±2.05% 7.3x
endOf quarter 159,346 ops/sec ±0.99% 1,165,512 ops/sec ±1.02% 7.3x
endOf year 165,736 ops/sec ±1.70% 2,026,810 ops/sec ±1.67% 12.2x

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 as 0000-03-01 instead.

New tests have been added to start_end_of.js that would have failed in earlier releases.

@icambron
Copy link
Member

icambron commented Dec 5, 2017

This looks great. Are these benchmarks from Moment's benchmarks suite or separate?

@ashsearle
Copy link
Contributor Author

From moment's benchmark suite - by running grunt release then the relevant benchmarks.

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);
Copy link
Member

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?

Copy link
Contributor Author

@ashsearle ashsearle Dec 6, 2017

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
)"

@icambron
Copy link
Member

icambron commented Dec 5, 2017

@ichernev this is a good one for you to look at

Copy link
Contributor

@ichernev ichernev left a 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.

/* falls through */
time = this._d.valueOf();
this._d.setTime(time - mod(time, MS_PER_MINUTE));
return this;
Copy link
Contributor

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

Copy link
Contributor Author

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.

if (units === 'quarter') {
this.month(Math.floor(this.month() / 3) * 3);
time = this._d.valueOf();
this._d.setTime(time - mod(time, MS_PER_SECOND));
Copy link
Contributor

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

use MS_PER_DAY

Copy link
Contributor Author

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.

case 'minute':
time = this._d.valueOf();
this._d.setTime(time - mod(time, MS_PER_MINUTE) + MS_PER_MINUTE - 1);
return this;
Copy link
Contributor

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

@ashsearle ashsearle closed this Jan 5, 2018
@ashsearle ashsearle reopened this Jan 5, 2018
@ashsearle
Copy link
Contributor Author

@ichernev Updated as requested. Do you have a specific issue in mind for the DST changes you think these changes fail to address?

@Havunen
Copy link

Havunen commented Mar 22, 2018

This PR indeed seems to fix issue DST startOf / endOf. Can this be merged?

@marwahaha
Copy link
Member

@ichernev - any thoughts?

@marwahaha
Copy link
Member

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 :-)

@marwahaha marwahaha dismissed ichernev’s stale review April 15, 2018 23:23

Changes are addressed

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;
Copy link
Member

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.

Copy link
Contributor Author

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 :-)


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');
Copy link
Member

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.)

Copy link
Contributor Author

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');
});
Copy link
Member

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.

Copy link
Contributor Author

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 94.803% when pulling 286fbe7 on ashsearle:fix/dst into 07d88ae on moment:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 94.803% when pulling 286fbe7 on ashsearle:fix/dst into 07d88ae on moment:develop.

@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage increased (+0.02%) to 88.143% when pulling 6abd6fd on ashsearle:fix/dst into 2d52ae7 on moment:develop.

@jarredt
Copy link

jarredt commented Nov 7, 2018

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...

@marwahaha
Copy link
Member

marwahaha commented Dec 13, 2018

Sorry for the delay! This will come in next release (not tonight though!)

src/lib/create/date-from-array.js Outdated Show resolved Hide resolved
src/lib/moment/start-end-of.js Outdated Show resolved Hide resolved
@marwahaha
Copy link
Member

Thanks @ashsearle - just a couple quick things and good to go!

@ashsearle
Copy link
Contributor Author

@marwahaha I've rebased this branch (to bring it up to date), and addressed your latest review comments.

@marwahaha marwahaha merged commit b4c343f into moment:develop Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants