1952623 - Generating current date using Temporal.Now.plainDateISO is 11x slower than generating random date using Temporal.PlainDate.from (year, month, day)

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, task, P3)

Description

[Mayank Bansal]

Attachment: Temporal date_PlainISODate.HTML

N=5000000
PlainISODate: https://share.firefox.dev/4bAZUIo (11s)
Random: https://share.firefox.dev/4i7w9S4 (1s)

Comment 1

[Mayank Bansal]

Attachment: Temporal date_random.HTML

Comment 2

[Mayank Bansal]

This is a very artificial testcase and Temporal is probbaly not on perf critical path. Feel free to WONTFIX :)

Comment 3

[Iain Ireland [:iain]]

Taking a quick look at the profile, we spend roughly half our time in icu_76::TimeZone::createTimeZone under GetOrCreateIntlTimeZone. We check for a cached intl::TimeZone object on the TimeZoneValue. However, the TimeZoneValue is freshly recreated each iteration by calling SystemTimeZone in SystemDateTime.

If we could cache the system time zone somewhere, I think we could avoid this. We would have to be sure that the cached time zone is still valid. If that's tricky to guarantee, maybe a one-item cache here would be good enough.

Anba, do you have any intuition about whether the performance of this code even matters?

Comment 4

[André Bargull [:anba]]

(In reply to Iain Ireland [:iain] from comment #3)

Anba, do you have any intuition about whether the performance of this code even matters?

Yes, I think it makes sense to ensure that this code doesn't run too slow.

Time zone caches for Temporal aren't yet implemented (bug 1840424), but at least for the system time zone case it should be possible to clone the time zone cached in js::DateTimeInfo.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1952623 - Part 1: Get system time zone offset from DateTimeInfo. r=#spidermonkey-reviewers!

Directly get the system time zone offset to avoid creating additional time
zone objects.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1952623 - Part 2: Combine GetISOPartsFromEpoch and BalanceISODateTime. r=#spidermonkey-reviewers!

Address the TODO note about combining GetISOPartsFromEpoch and BalanceISODateTime.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1952623 - Part 3: Avoid unnecessary to nanoseconds conversion. r=mgaudet!

Drive-by change:

• SystemUTCEpochNanoseconds is infallible, so we can return the result
  directly and don't need an out-param.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c1a710f1cd8
Part 1: Get system time zone offset from DateTimeInfo. r=spidermonkey-reviewers,mgaudet
https://hg.mozilla.org/integration/autoland/rev/e113ac96f039
Part 2: Combine GetISOPartsFromEpoch and BalanceISODateTime. r=spidermonkey-reviewers,mgaudet
https://hg.mozilla.org/integration/autoland/rev/225df4261b1c
Part 3: Avoid unnecessary to nanoseconds conversion. r=mgaudet

Comment 9

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/5c1a710f1cd8
https://hg.mozilla.org/mozilla-central/rev/e113ac96f039
https://hg.mozilla.org/mozilla-central/rev/225df4261b1c

Comment 10

[Mayank Bansal]

N=5000000
PlainISODate: https://share.firefox.dev/4jgRCsc (600ms)
Random: https://share.firefox.dev/4czHAAe (1.1s)

So we are now 18x faster!

Comment 11

[Mayank Bansal]

i am guessing that this should go to release at the same time as Temporal?

Comment 12

[André Bargull [:anba]]

Yes, this will land in the same release as bug 1912511.