perf: speed up date formatting by 555x

[kevcodez]

The current version of date formatting is constructing a bunch of new objects using Intl and is comparetively slow. The simply toISOString and splice show a local speed up of up to 300x in terms of formatting and should yield the same results.

The date tests use momentjs formatting as comparison, which seems to respect time zones, thus, I also respected the time zones.

Before change

fast-json-stringify date format x 2,900 ops/sec ±1.19% (82 runs sampled)

After change

fast-json-stringify date format x 1,608,371 ops/sec ±1.30% (89 runs sampled)

x 555 faster

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[zekth]

Indeed good catch:

fast-json-stringify/index.js

Lines 313 to 316 in e0d9336

	function $asString (str, skipQuotes) {
	const quotes = skipQuotes === true ? '' : '"'
	if (str instanceof Date) {
	return quotes + str.toISOString() + quotes

Could you include benchmarks?

[kevcodez]

@zekth Added a benchmark for date formatting

[mcollina]

lgtm

[Eomm]

CI is failing

2021-09-14T16:00:05.9090566Z             --- expected
2021-09-14T16:00:05.9090925Z             +++ actual
2021-09-14T16:00:05.9091357Z             @@ -1,1 +1,1 @@
2021-09-14T16:00:05.9091956Z             -"1975-08-19"
2021-09-14T16:00:05.9092379Z             +"1975-08-18"

My TZ is GMT+2, it should be related

[kevcodez]

The result from the current implementation is equal to what moment(toStringify).format('YYYY-MM-DD') would output (the other date tests use this).

Instantiating a new Date automatically converts it to UTC.

getTimezoneOffset gets the offset compared to the machine running the code, thus yielding different results.

The current implementation takes that into account to always output the UTC date and not put out any system-specific dates (otherwise the other tests fail locally) - this is the same behavior as moment.js.

Tbh, I'd keep this test out and keep the implementation as is.

WDYT?

You can see in this screenshot, when running the Node docker image, the timezone gets lost and there is no way to access it from a newly instantiated Date. The previous implementation (using Intl did in fact output day 18 instead of 19, too - so the desired behavior of outputting day 19 would be a breaking change).

[zekth]

CI is failing

2021-09-14T16:00:05.9090566Z             --- expected
2021-09-14T16:00:05.9090925Z             +++ actual
2021-09-14T16:00:05.9091357Z             @@ -1,1 +1,1 @@
2021-09-14T16:00:05.9091956Z             -"1975-08-19"
2021-09-14T16:00:05.9092379Z             +"1975-08-18"

My TZ is GMT+2, it should be related

That's what was wondering. We need to enforce UTC ain't we?

[kevcodez]

@zekth From your comment

console.log(s(new Date('August 19, 1975 00:15:30 GMT+01:00'))) // "1975-08-19"

I thought, you want to the result of the serialization from the given date to be 1975-08-19, which wouldn't be UTC and would be different to the current Intl implementation.

Do you want to remove that new test or do you want the assertion to be 1975-08-18?

[zekth]

Assertion shouldn't be changed. Atm i have no proper solution that comes to my mind.

[kevcodez]

Again, this would break the current implementation. It does not work with JS dates like that. That assertion would also break with the current Intl implementation. The test would fail without my changes given that assertion. I don't think we are trying to introduce a breaking change here.

If you want dates in a specific time zone, you can use libraries like date-fns-tz, but it can't be handled in here.

[mcollina]

I'm a bit lost on the next steps here.

[kevcodez]

@mcollina Not sure how this conversation got lost.

@Eomm requested an assertion that is not possible to implement or make it system-independent (independent of local timezone). The requested assertion would also fail without my changes. So in my opinion, the requested assertion is not correct and cannot/should not be solved in this PR. Making this assertion work, would actually be a breaking change. My changes are not meant to break/change the behavior of date formatting, but only increase the performance. The same results are output.

When instantiating a new date in JS, the date is automatically converted to UTC and there is no way to access the initial timezone information afterwards. So it is not possible to make this assertion work like that.

My suggestion is, to remove that requested test (because it does not make sense IMO) and keep it that way.

[zekth]

@kevcodez sorry i misinterpreted the test case added. Indeed it's LGTM to me.

good catch on this

[climba03003]

I have look into the document

Intl.DateTimeFormat will respect the system timezone offset. Is it not the same case in NodeJS?

[kevcodez]

@climba03003 The new implementation respects the system timezone offset using getTimezoneOffset. However, when instantiating a new Date, Node automatically converts it to UTC and you cannot get the initial timezone. You can convert the UTC date to your system timezone (which my change does, just like Intl), but you cannot access the initial timezone from the instantiated date.

[climba03003]

LGTM. I didn't notice the handling is updated. Currently, it should respect the system timezone when it is Date object.

[kevcodez]

@climba03003 Intl and getTimezoneOffset both use your system time. Just like the proposed changes in this PR. This behavior is unchanged.

When running this on a system with UTC time, i.e. the GitHub action, this will have 0 as timezone offset and will output the day 18. So, we cannot have a test like this

const date = new Date('August 19, 1975 00:15:30 GMT+01:00')
const output = stringify(date)
  t.equal(output, '1975-08-19')

This will only work on systems that are in a GMT+ or above timezone, but will fail on any system with UTC or before timezone.

The current tests compare the output with what momentjs.format would do, which does respect the system timezone. Meaning, both Intl and the proposed new implementation respect the system timezone.

[mcollina]

lgtm

[kevcodez]

@mcollina it would be super dope if you could release the changes when you find some time 🙏

[mcollina]

I'll get to it once we finish restoring the array perf.