perf(datetime): toISO 100x faster

[gpaucot]

toISO is very slow compared to the competition: 80x slower than dayjs, 40x slower than momentjs and 50x slower than date-fns.

This implementation makes it 100x faster than before or 125% faster than dayjs.

Please merge 🙂

[linux-foundation-easycla]

• ❌ The commit (bd23e4d) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[linux-foundation-easycla]

• ❌ The commit (bd23e4d) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[gpaucot]

CLA fixed 🙂

[icambron]

Thanks, I've been considering this optimization for a while. A few thoughts:

1. We don't necessarily have to support every option for toISO with this "manual" version. We could do it only for the defaults and fallback if the user wants a variant. That would save us some code. Always trying to balance bundle size with perf here.
2. How does this compare perfwise with dt.toJSDate().toISOString()? The main disadvantage of the "native" method is that it always returns the string in UTC. But if it were significantly faster, we could check the zone and call that method conditionally
3. Was using the simplified padStart a major perf improvement?
4. Stylistically, I'd prefer we create all the strings in individual variables and then drop them all into a format string, like ${y}-${M}-..., unless that has a major performance consequence

Edited to add:

5. It doesn't look like it handles years with more than 4 digits. (There don't seem to be tests for this?). The year 123456 gets written as +123456. So I think you need both the "unsimplified" padStart and some logic to drop in the +
6. It doesn't handle negative years either, since padStart would cut off the minus sign

(I haven't tested those, but I think that's right...)

[icambron]

For fun, I ported this to my experimental fork of Luxon and found a few things:

• The reason the tests aren't helping you as much as they should is that the tests rely on the fact that toISO composes toISODate and toISOTime and tests those individually. Since you left them as-is, you're not getting the benefit of the tests
• In addition, it means those two functions don't get any perf improvements
• We should switch the padStart impl to use the native string.prototype.padStart. That's what I did here
• Using your simplified padStart works fine for everything but years, which can be longer and/or negative

Here's what I ended up with (luxon-next is similar code but arranged completely differently): https://github.com/icambron/luxon-next/blob/451129e297c8c7fc8a2501abbd41deda1e9075e3/src/dateTime/format.ts#L203

I haven't benchmarked yet, so maybe there's more to do there.

[gpaucot]

Hi Isaac, thanks for the review! I take one point at a time:

From your first message:

1. Yes, it could be for a future PR, at this time I don't change the signature, just try some perf improvements :-)
2. Exactly, actually it is significantly faster more than 2x as fast... however to use it we should check that we are extended, suppressSeconds = false, suppressMilliseconds = false, includeOffset = true, allowZ = true and offset = 0 which is the case for very few users around the world, checking for everyone would reduce the perf for almost everyone.
3. Yes ~15% but by switching padStart to string.prototype.padStart, we keep the gain. The perf are better doing ('' + year) than year.toString(), that is what you will see in the PR
4. Historically += was the fastest way to concatenate string, today +, += and template string look even so wathever makes it more readable will be best
   5 & 6. Fixed, thanks !

From the second one:

The reason the tests aren't helping you as much as they should is that the tests rely on the fact that toISO composes toISODate and toISOTime and tests those individually. Since you left them as-is, you're not getting the benefit of the tests

You were right, I was missing tests, it should be better now

In addition, it means those two functions don't get any perf improvements

True, step by step ^^

We should switch the padStart impl to use the native string.prototype.padStart

Done, it changed your implementation slightly to use ('' + v) instead of v.toString() as it is faster

Using your simplified padStart works fine for everything but years, which can be longer and/or negative

Fixed

[icambron]

I pulled your branch and added some commits. See #1108. Closing this PR and we can continue over there.