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.

Sign up for GitHub

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

Jump to bottom

Use strings in slots to optimize builtin calendar and time zone implementations #2482

Merged
merged 14 commits into from
Apr 7, 2023
Merged

Use strings in slots to optimize builtin calendar and time zone implementations #2482

merged 14 commits into from
Apr 7, 2023

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jan 19, 2023 •
edited by justingrant
Loading

This is the change discussed in #1808, to be discussed in the Temporal champions meeting.

This is a large breaking change requested by implementers. It optimizes implementation performance by removing the requirement to create new Temporal.Calendar/Temporal.TimeZone instances in the 99%+ case of using built-in calendars and time zones. Instead, only the string IDs of those built-in calendars or time zones will be stored in the internal slots of Temporal objects like Temporal.ZonedDateTime that today store calendar and/or time zone objects in internal slots.

@ptomato ptomato force-pushed the 1808-string-builtin-calendar branch from e87cca2 to c1ebf65 Compare January 19, 2023 19:46
@ptomato
Copy link
Collaborator Author

ptomato commented Jan 19, 2023 •
edited
Loading

To do after today's discussion:

  • Change toJSON() to not call toString()
  • Change builtin dateFromFields()/yearMonthFromFields()/monthDayFromFields()/dateAdd() to always put a string in the returned object's [[Calendar]] internal slot
  • Fix linter issues
  • Update cookbook examples
  • Update valid strings test

@gibson042 gibson042 mentioned this pull request Jan 19, 2023
Closed
@ptomato ptomato mentioned this pull request Jan 20, 2023
Merged
@justingrant justingrant mentioned this pull request Jan 20, 2023
Closed
@justingrant justingrant changed the title Draft: Strings for builtin calendars and time zones Draft: Use strings in slots to optimize builtin calendar and time zone implementations Jan 20, 2023
@justingrant justingrant mentioned this pull request Jan 20, 2023
Merged
@ptomato ptomato force-pushed the 1808-string-builtin-calendar branch from c1ebf65 to 106b1a5 Compare January 21, 2023 01:13
@ptomato ptomato mentioned this pull request Jan 31, 2023
Closed
@bakkot bakkot mentioned this pull request Feb 8, 2023
Closed
@codecov
Copy link

codecov bot commented Feb 16, 2023 •
edited
Loading

Codecov Report

Merging #2482 (dd150fe) into main (c6c513d) will increase coverage by 0.01%.
The diff coverage is 97.84%.

@@            Coverage Diff             @@
##             main    #2482      +/-   ##
==========================================
+ Coverage   95.89%   95.91%   +0.01%     
==========================================
  Files          20       20              
  Lines       10938    11104     +166     
  Branches     2046     2112      +66     
==========================================
+ Hits        10489    10650     +161     
- Misses        388      392       +4     
- Partials       61       62       +1
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.19% <96.27%> (-0.10%) ⬇️
polyfill/lib/calendar.mjs 87.79% <100.00%> (+0.25%) ⬆️
polyfill/lib/instant.mjs 96.46% <100.00%> (-0.02%) ⬇️
polyfill/lib/intl.mjs 95.16% <100.00%> (-0.10%) ⬇️
polyfill/lib/now.mjs 100.00% <100.00%> (ø)
polyfill/lib/plaindate.mjs 99.67% <100.00%> (+<0.01%) ⬆️
polyfill/lib/plaindatetime.mjs 97.25% <100.00%> (+0.02%) ⬆️
polyfill/lib/plainmonthday.mjs 97.76% <100.00%> (+0.08%) ⬆️
polyfill/lib/plaintime.mjs 98.23% <100.00%> (-0.04%) ⬇️
polyfill/lib/plainyearmonth.mjs 98.40% <100.00%> (+0.04%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Replace Temporal.Now.timeZone → Temporal.Now.timeZoneId
bc6a4dc 
Now.timeZoneId() returns a string so that it can be used to construct
objects that have the builtin time zone behaviour.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Remove calendar slot from Temporal.PlainTime
6b1b500 
Has several effects on existing tests:
- Remove PlainTime from various tests that extract a Temporal.Calendar
  instance from another Temporal object.
- Remove Temporal.PlainTime.prototype.calendar property.
- Remove calendar property from object returned from
  Temporal.PlainTime.prototype.getISOFields().
- Ignore calendar annotation when converting ISO string to PlainTime.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Replace Temporal objects' .calendar getters with .calendarId/.getCale…
4dda712 
…ndar

This is the replacement of the old API with the new API. Semantics will be
corrected in the following commit.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Store strings or objects in Temporal objects' [[Calendar]] slot
3204783 
In several tests involving custom calendars, we need to change the
implementation of dateFromFields/monthDayFromFields/yearMonthFromFields so
that the returned object gets the receiver as its calendar after chaining
up to the builtin implementation.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Change Temporal.Calendar compare semantics to use .id
6c8c3d8 
Compare semantics for custom calendars that _don't_ extend
Temporal.Calendar (and therefore don't have the internal slot) use the
value of the .id property, instead of calling toString().

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Change Temporal.Calendar.prototype.toJSON not to call toString
1251e06 
Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Replace Temporal.ZonedDateTime.timeZone getter with new API
6c71cf6 
This is the replacement of the old API with the new API, .timeZoneId and
.getTimeZone(). Semantics will be corrected in the following commit.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Store strings or objects in Temporal.ZonedDateTime [[TimeZone]] slot
1b8c60a 
Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Change Temporal.TimeZone compare semantics to use .id
b0260ef 
Compare semantics for custom time zones that _don't_ extend
Temporal.TimeZone (and therefore don't have the internal slot) use the
value of the .id property, instead of calling toString().

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 17, 2023
@ptomato
Change Temporal.TimeZone.prototype.toJSON not to call toString
9b303c8 
Normative PR: tc39/proposal-temporal#2482
@ptomato ptomato mentioned this pull request Feb 17, 2023
Merged
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 17, 2023

This change now has coverage in test262: tc39/test262#3788

ptomato added a commit to ptomato/test262 that referenced this pull request Feb 18, 2023
@ptomato
Replace Temporal.Now.timeZone → Temporal.Now.timeZoneId
d4a5ee8 
Now.timeZoneId() returns a string so that it can be used to construct
objects that have the builtin time zone behaviour.

Normative PR: tc39/proposal-temporal#2482
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 18, 2023
@ptomato
Remove calendar slot from Temporal.PlainTime
85637f8 
Has several effects on existing tests:
- Remove PlainTime from various tests that extract a Temporal.Calendar
  instance from another Temporal object.
- Remove Temporal.PlainTime.prototype.calendar property.
- Remove calendar property from object returned from
  Temporal.PlainTime.prototype.getISOFields().
- Ignore calendar annotation when converting ISO string to PlainTime.

Normative PR: tc39/proposal-temporal#2482
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 10: Don't call ToString in Temporal.TimeZone.proto…
39b2400 
…type.toJSON. r=spidermonkey-reviewers,sfink

Also per <tc39/proposal-temporal#2482>

Depends on D182027

Differential Revision: https://phabricator.services.mozilla.com/D182028

UltraBlame original commit: 1499a730b4bf448ff87289899b462eaadd92bc36
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 11: Don't call ToString in Temporal.Calendar.proto…
e4d72e6 
…type.toJSON. r=spidermonkey-reviewers,dminor

Also per <tc39/proposal-temporal#2482>.

Depends on D182028

Differential Revision: https://phabricator.services.mozilla.com/D182029

UltraBlame original commit: 06d1f2c99d3655bb48f32c40284c4b676d578f5c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 12: Implement ToTemporalCalendarIdentifier abstrac…
4d14775 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182029

Differential Revision: https://phabricator.services.mozilla.com/D182030

UltraBlame original commit: e85fd6da2df853727ec62267c18fd2948b0e08d0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 13: Implement ToTemporalTimeZoneIdentifier abstrac…
00169c8 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182030

Differential Revision: https://phabricator.services.mozilla.com/D182031

UltraBlame original commit: 7c449d1bc88f0c7656dc99ac3982f10b44ac312f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 14: Replace calendar and timeZone accessors on Tem…
39f59c5 
…poral objects. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182031

Differential Revision: https://phabricator.services.mozilla.com/D182032

UltraBlame original commit: 5bf5631f760c7672d7541e838805b2e5aa5a49df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 16: Add support for string calendars. r=spidermonk…
9295ca2 
…ey-reviewers,allstarschh

`CalendarValue` now holds either a calendar object (`CalendarObject` or any
`JSObject` for user-calendars) or a calendar identifier string. This isn't
necessarily the final design for `CalendarValue`, but works for now until we
add support more built-in calendars beside the ISO-8601 calendar.

Per <tc39/proposal-temporal#2482>

Differential Revision: https://phabricator.services.mozilla.com/D182034

UltraBlame original commit: 193dea45a684255439b5532696b4df3cdc1b96f7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 17: Replace Temporal.Now.timeZone with Temporal.No…
bf02ebd 
…w.timeZoneId. r=spidermonkey-reviewers,allstarschh

Replace the function per <tc39/proposal-temporal#2482>.

Differential Revision: https://phabricator.services.mozilla.com/D182035

UltraBlame original commit: b4fe5032e88b5b06047c9781d75774e56fc527c9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 19: Rename RejectObjectWithCalendarOrTimeZone to R…
f845170 
…ejectTemporalLikeObject. r=spidermonkey-reviewers,allstarschh

Rename per <tc39/proposal-temporal#2482>.

Depends on D182036

Differential Revision: https://phabricator.services.mozilla.com/D182038

UltraBlame original commit: 4bb3d334eaacf0084e325b1b85d1352f9fd44051
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 5: Remove calendar from PlainTime. r=spidermonkey-…
0a2a7c3 
…reviewers,mgaudet

PlainTime objects no longer have an associated calendar slot.

See <tc39/proposal-temporal#2482>.

Depends on D182022

Differential Revision: https://phabricator.services.mozilla.com/D182023

UltraBlame original commit: ece2a09b9b46acf5245b5e04f96a79a3ed3c9c3e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 10: Don't call ToString in Temporal.TimeZone.proto…
2cf790f 
…type.toJSON. r=spidermonkey-reviewers,sfink

Also per <tc39/proposal-temporal#2482>

Depends on D182027

Differential Revision: https://phabricator.services.mozilla.com/D182028

UltraBlame original commit: 1499a730b4bf448ff87289899b462eaadd92bc36
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 11: Don't call ToString in Temporal.Calendar.proto…
b1cb0ed 
…type.toJSON. r=spidermonkey-reviewers,dminor

Also per <tc39/proposal-temporal#2482>.

Depends on D182028

Differential Revision: https://phabricator.services.mozilla.com/D182029

UltraBlame original commit: 06d1f2c99d3655bb48f32c40284c4b676d578f5c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 12: Implement ToTemporalCalendarIdentifier abstrac…
acda3bc 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182029

Differential Revision: https://phabricator.services.mozilla.com/D182030

UltraBlame original commit: e85fd6da2df853727ec62267c18fd2948b0e08d0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 13: Implement ToTemporalTimeZoneIdentifier abstrac…
e46c429 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182030

Differential Revision: https://phabricator.services.mozilla.com/D182031

UltraBlame original commit: 7c449d1bc88f0c7656dc99ac3982f10b44ac312f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 14: Replace calendar and timeZone accessors on Tem…
bd362f3 
…poral objects. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182031

Differential Revision: https://phabricator.services.mozilla.com/D182032

UltraBlame original commit: 5bf5631f760c7672d7541e838805b2e5aa5a49df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 16: Add support for string calendars. r=spidermonk…
d057637 
…ey-reviewers,allstarschh

`CalendarValue` now holds either a calendar object (`CalendarObject` or any
`JSObject` for user-calendars) or a calendar identifier string. This isn't
necessarily the final design for `CalendarValue`, but works for now until we
add support more built-in calendars beside the ISO-8601 calendar.

Per <tc39/proposal-temporal#2482>

Differential Revision: https://phabricator.services.mozilla.com/D182034

UltraBlame original commit: 193dea45a684255439b5532696b4df3cdc1b96f7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 17: Replace Temporal.Now.timeZone with Temporal.No…
6af7aac 
…w.timeZoneId. r=spidermonkey-reviewers,allstarschh

Replace the function per <tc39/proposal-temporal#2482>.

Differential Revision: https://phabricator.services.mozilla.com/D182035

UltraBlame original commit: b4fe5032e88b5b06047c9781d75774e56fc527c9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 19: Rename RejectObjectWithCalendarOrTimeZone to R…
edce36a 
…ejectTemporalLikeObject. r=spidermonkey-reviewers,allstarschh

Rename per <tc39/proposal-temporal#2482>.

Depends on D182036

Differential Revision: https://phabricator.services.mozilla.com/D182038

UltraBlame original commit: 4bb3d334eaacf0084e325b1b85d1352f9fd44051
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 5: Remove calendar from PlainTime. r=spidermonkey-…
7b6784c 
…reviewers,mgaudet

PlainTime objects no longer have an associated calendar slot.

See <tc39/proposal-temporal#2482>.

Depends on D182022

Differential Revision: https://phabricator.services.mozilla.com/D182023

UltraBlame original commit: ece2a09b9b46acf5245b5e04f96a79a3ed3c9c3e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 10: Don't call ToString in Temporal.TimeZone.proto…
e59d098 
…type.toJSON. r=spidermonkey-reviewers,sfink

Also per <tc39/proposal-temporal#2482>

Depends on D182027

Differential Revision: https://phabricator.services.mozilla.com/D182028

UltraBlame original commit: 1499a730b4bf448ff87289899b462eaadd92bc36
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 11: Don't call ToString in Temporal.Calendar.proto…
95ac47e 
…type.toJSON. r=spidermonkey-reviewers,dminor

Also per <tc39/proposal-temporal#2482>.

Depends on D182028

Differential Revision: https://phabricator.services.mozilla.com/D182029

UltraBlame original commit: 06d1f2c99d3655bb48f32c40284c4b676d578f5c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 12: Implement ToTemporalCalendarIdentifier abstrac…
de966d7 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182029

Differential Revision: https://phabricator.services.mozilla.com/D182030

UltraBlame original commit: e85fd6da2df853727ec62267c18fd2948b0e08d0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 13: Implement ToTemporalTimeZoneIdentifier abstrac…
caa3bc7 
…t operation. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182030

Differential Revision: https://phabricator.services.mozilla.com/D182031

UltraBlame original commit: 7c449d1bc88f0c7656dc99ac3982f10b44ac312f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 14: Replace calendar and timeZone accessors on Tem…
97441bb 
…poral objects. r=spidermonkey-reviewers,dminor

From <tc39/proposal-temporal#2482>.

Depends on D182031

Differential Revision: https://phabricator.services.mozilla.com/D182032

UltraBlame original commit: 5bf5631f760c7672d7541e838805b2e5aa5a49df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 16: Add support for string calendars. r=spidermonk…
3f5af46 
…ey-reviewers,allstarschh

`CalendarValue` now holds either a calendar object (`CalendarObject` or any
`JSObject` for user-calendars) or a calendar identifier string. This isn't
necessarily the final design for `CalendarValue`, but works for now until we
add support more built-in calendars beside the ISO-8601 calendar.

Per <tc39/proposal-temporal#2482>

Differential Revision: https://phabricator.services.mozilla.com/D182034

UltraBlame original commit: 193dea45a684255439b5532696b4df3cdc1b96f7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 17: Replace Temporal.Now.timeZone with Temporal.No…
9c3a279 
…w.timeZoneId. r=spidermonkey-reviewers,allstarschh

Replace the function per <tc39/proposal-temporal#2482>.

Differential Revision: https://phabricator.services.mozilla.com/D182035

UltraBlame original commit: b4fe5032e88b5b06047c9781d75774e56fc527c9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
@marco-c
Bug 1840374 - Part 19: Rename RejectObjectWithCalendarOrTimeZone to R…
df9796f 
…ejectTemporalLikeObject. r=spidermonkey-reviewers,allstarschh

Rename per <tc39/proposal-temporal#2482>.

Depends on D182036

Differential Revision: https://phabricator.services.mozilla.com/D182038

UltraBlame original commit: 4bb3d334eaacf0084e325b1b85d1352f9fd44051
This was referenced Aug 11, 2023
Merged
Merged
ptomato added a commit that referenced this pull request Nov 3, 2023
@ptomato
Editorial: Fix type of [[TimeZone]] and [[Calendar]] internal slots
c69a2db 
Since #2482, these can be strings as well as objects.

Thanks to Ms2ger for catching this.

Closes: #2714
Ms2ger pushed a commit that referenced this pull request Nov 3, 2023
@ptomato @Ms2ger
Editorial: Fix type of [[TimeZone]] and [[Calendar]] internal slots
df567ef 
Since #2482, these can be strings as well as objects.

Thanks to Ms2ger for catching this.

Closes: #2714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@justingrant justingrant justingrant approved these changes

@gibson042 gibson042 gibson042 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ptomato @justingrant @ljharb @gibson042