Speed up toLocaleString ~2.5x by reducing creation of Intl.DateTimeFormat instances

[justingrant]

This PR speeds up toLocaleString and other operations that depend on the polyfilled Intl.DateTimeFormat. The fix was to prevent unnecessary creation of built-in Intl.DateTimeFormat objects, because the constructor of that built-in class is slooooooow. For more details about the underlying issue see: https://bugs.chromium.org/p/v8/issues/detail?id=6528

In local testing, speedup is about 2.5x for ZDT toLocaleString calls. The sample code below runs in about 1800ms in the playground of the current main branch, and about 700ms in the playground of the PR branch.

(() => {
  start = Date.now();
  for (i = 0; i < 1000; i++) {
    Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toLocaleString();
  }
  total = Date.now()-start;
  console.log(`${total}ms`);
}) ()

[ptomato]

Thanks, this looks good!

[ptomato]

options would no longer be undefined here unless the caller explicitly passed undefined, I think? Maybe we should have options = undefined as the function default argument. It doesn't really matter much though. I believe this is correct as is.

[justingrant]

Yep, I made exactly this change in my latest checkin. I also realized that shallow-cloning of options was not enough, because if options property values or locale were not primitives, then they could also be mutated by the caller before lazy-init happens. Take a look at my last checkin and let me know if the hacky JSON-based deep clone is sufficient.

[justingrant]

I also added tests to verify that mutating constructor inputs after the constructor returns won't break anything.

Still not sure that using a JSON round-trip is the right way to deep-clone these inputs.

[ptomato]

Similarly to you, I am feeling a bit iffy about the deep-cloning, but maybe it can be avoided altogether?

[ptomato]

Would it be possible to avoid the deep cloning altogether, and store ro instead of options for lazily creating new instances? (and use ro.locale as the locale?)

I ask because either deep- or shallow-cloning the object is going to be very tricky to do while matching the observable behaviour in ToDateTimeOptions. But it seems like no matter whether we create a DateTimeFormat with the originally-given or the resolved options/locale, it should have the same result.

[justingrant]

@ryzokuken @sffc - @ptomato's idea above sounds like a good one. Will there be any difference in behavior between i1 and i2 below?

i1 = new DateTimeFormat(locale, options);
ro = i1.resolvedOptions();
i2 = new DateTimeFormat(ro.locale, ro);

[justingrant]

Update: using resolvedOptions() output to replace options didn't work, because the polyfill adds additional options if the user's options don't contain unit options like year or hour.

What does seem to work is a small variation of this approach: clone resolvedOptions()and delete all properties that weren't present in the original options object.

@ptomato - take a look at my latest commit and LMK if you think this is a promising approach.

[ptomato]

I think this solution is good, though maybe one of the Intl folks could chip in?

[ptomato]

I would use ES.HasOwnProperty() here, to avoid calling getters a second time. That will still not quite be spec-compliant, since Proxy objects would be able to observe the HasProperty operation, but it's probably fine. Although that might mean we shouldn't backport this to proposal-temporal.

[justingrant]

I would use ES.HasOwnProperty() here, to avoid calling getters a second time. That will still not quite be spec-compliant, since Proxy objects would be able to observe the HasProperty operation, but it's probably fine. Although that might mean we shouldn't backport this to proposal-temporal.

@ptomato I looked at the current polyfill code and I think that it has the multi-getter-call problem too. Look at the amend function. I don't think the current polyfill has the Proxy problem because it ObjectAssigns the options object before doing anything with it.

Anyway, I just pushed a new commit that does the options cloning (like the old polyfill) before reading, and also uses ES.HasOwnProperty() to avoid making the proxy multi-getter problem any worse than it already is. I'll leave it for someone else to fix amend later. Is this OK?

Also, I think this should be safe for back-porting because it doesn't make the current polyfill worse. What do you think?

LMK if you (or @ryzokuken @sffc) have any ideas how to improve this further.

[Ms2ger]

Yeah, I was planning to take a look at the handling in the spec polyfill at some point. I guess we can accept this as "don't pass silly arguments" here, though.

[justingrant]

I think this PR is ready to merge, but I'm going to hold off on merging it for a day or so so that we can port and merge the now=>Now rename (see tc39/proposal-temporal#1645) first. As soon as that's merged in the old repo, I'll prepare a port of it for this repo.