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.

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

[[Calendar]] in Temporal.PlainTime and GetISO8601Calendar() #1588

Closed
FrankYFTang opened this issue Jun 29, 2021 · 16 comments
Closed

[[Calendar]] in Temporal.PlainTime and GetISO8601Calendar() #1588

FrankYFTang opened this issue Jun 29, 2021 · 16 comments
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Jun 29, 2021

Currently, in Temporal.PlainTime Instances
https://tc39.es/proposal-temporal/#sec-properties-of-temporal-plaintime-instances
[[Calendar]] | An instance of the built-in ISO 8601 calendar.

and
https://tc39.es/proposal-temporal/#sec-temporal-createtemporaltime
11. Set object.[[Calendar]] to ? GetISO8601Calendar().

So... what is the value for the Temporal.PlainTime carry this internal slot if the slot is always getting the same kind of calendar (but different object) and the calendar internally carry no state information. It force the implementation to use more memory than it needed to and provide no value.

from what I can I read the spec mandate us to make sure the following

let a = new Temporal.PlainTime(....)
let b = new Temporal.PlainTime(....)
# different Temporal.PlainTime must return different object of Temporal.Calendar)
# so my implementation can NOT cache one Temporal.Calendar object per identifier and re use it
# for all reference to that "iso8601" calendar.
a.calendar == b.calendar => must be false

# different calls to the get Temporal.PlainTime.prototype.calendar must return the same Temporal.Calendar object
# so my internal implementation can NOT choose not to create the calendar in the constructor but only create one
# in the get Temporal.PlainTime.prototype.calendar
a.calendar == a.calendar => must be true

Please forgive me if I misunderstand the interpretation of the spec. I try to figour out how to optimize the memory usage for PlainTime. My first intutiion is that since the calendar is always iso8601 there are no need to create on and store it into the internal slot and we could skip a pointer in the PlainTime internal storage and always call GetISO8601Calendar() to return a new copy in the getter of calendar.

@ptomato
Copy link
Collaborator

ptomato commented Jun 29, 2021

I think what you said in the # comments is correct: you can choose not to create the calendar in the constructor, and only create it in the .calendar getter (or in .getISOFields().) But the final paragraph I don't think is correct; once it's created, you have to store it. (This is closely related to #1587 as well.)

@FrankYFTang
Copy link
Contributor Author

The problem I don't understand is, if the calendar in the internal slot of PlainTime is always iso8601 calendar, then why we need to create one and store in it's internal slot? We do we not to create it when we really need to have one but force to always create on in the PlainTime constructor and carry it around, even it is just a "constant" in the system.

@ptomato
Copy link
Collaborator

ptomato commented Jun 30, 2021

I don't think it's forced to be created in the PlainTime constructor, since that creation is not observable.

@ljharb
Copy link
Member

ljharb commented Jun 30, 2021

Creating it in the constructor unobservably, though, is a strong signal to implementors that the semantics must be as if it's created there.

@ptomato
Copy link
Collaborator

ptomato commented Jun 30, 2021

I agree, and additionally I believe that agrees with all but the final paragraph of the OP.

@FrankYFTang
Copy link
Contributor Author

What I do not understand is why do this object carry a calendar if it is not used by the object and it is always iso8601? It seems forcing the implementation to waste memory for nothing. Even if we somehow lazy create that, the spec make sure the PlainTime need to reserve a reference to the created one and always return the same one later- the fact the PlainTime need to have a place to remeber what got return the first time and always return the same one later while everyone of them which got return is the same kind is really a waste and inefficency.

@FrankYFTang
Copy link
Contributor Author

@sffc

@FrankYFTang
Copy link
Contributor Author

@justingrant @sffc @ryzokuken
I propose we remove calendar from PlainTime internal slot, as well as the get Temporal.PlainTime.prototype.calendar. It does not make sense to create a constant in the object and return the same calendar every time and do not use it at all. For all the places which need to access PlainTime.prototype.calendar, the code can just be changed to to call GetISO8601Calendar() to create an new object.
Also remove it from 4.3.19 Temporal.PlainTime.prototype.getISOFields ( )

.

@ptomato
Copy link
Collaborator

ptomato commented Jul 2, 2021

PlainTime carries a calendar because we believed it is necessary, if we ever want to add time calendars without breaking the web. See #522 and https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-11-12.md#time-calendars for more context.

@FrankYFTang
Copy link
Contributor Author

If we truly believe it is necessary to carries a calendar, then we should allow the constructor to pass in one and not mandate the internal slot to always carry an ISO8601? How we would not breaking the web years later if all the code on the web already assume the one return by PlainTime is always ISO8601 and then somehow it all of sudden start to return some other calendar. In that sense, it will still break the web, right?

@sffc
Copy link
Collaborator

sffc commented Jul 4, 2021

It was mainly a discussion about the semantics of ... spreading, and wanting to make ... spreads contain a calendar field to be more future-proof. @pipobscure can add more clarity.

@justingrant
Copy link
Collaborator

@sffc Didn't we remove spreading use-cases when we removed fields before Stage 3?

@ptomato
Copy link
Collaborator

ptomato commented Jul 5, 2021

Since PlainTime instances can no longer be spread, does that remove the break-the-web concern? It at least removes the concern about plainDateTime.with(...plainTime) because that already didn't work anymore. But I seem to remember there were other concerns?

How we would not breaking the web years later if all the code on the web already assume the one return by PlainTime is always ISO8601 and then somehow it all of sudden start to return some other calendar. In that sense, it will still break the web, right?

This is also a concern for PlainDate etc. Unfortunately a lot of code will assume these are also always ISO 8601 and that's why we have all of these mitigations as described in The Unexpected Calendar Problem. This is a separate problem from the concern about time-aware calendars breaking the web.

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Jul 8, 2021

sorry, I have no idea what is " ... spreading" . My point is if we need to make a decision here, either

  1. we believe Calendar is necessary in PlainTime then we should just let the constructor to take one and store it internally, instead of always. return a ISO8601 one uncondictionally, OR
  2. we blieve Calendar is unnecessary in PlainTime then therefore there are no need to create a constant one ISO8601 internally and hold on to it and return to the same object on the get (I don't mind we return a different ISO8601 each time we call the getter).

I think currently the requirement of the combination of

  1. not allow a pass in a calendar AND
  2. mandate the get Temporal.PlainTime.prototype.calendar always return the SAME ISO8601 calendar (which mean, each Temporal.PlainTime object need a place (at least a pointer) to store that) for the same Temporal.PlainTime object, and
  3. mandate different get Temporal.PlainTime.prototype.calendar have to return different ISO8601 calendar (because it is create inside Temporal.PlainTime so two Temporal.PlainTime cannot return the same global cached ISO8601 calendar on their get Temporal.PlainTime.prototype.calendar ) from two different Temporal.PlainTime object
    is strange and wasteful of computing resources. (I need to reserve at least 64 bits as a pointer to a Calendar each Temporal.PlainTime object internally to keep track of that) This bump my memory requirement for each Temporal.PlainTime object from 8 bytes to 16 bytes (see https://docs.google.com/document/d/1Huu2OUlmveBh4wjgx0D7ouC9O9vSdiZWaRK3OwkQZU0/edit#heading=h.qgugoko2f2wb) a 100% increase for NOTHING. I do not mind to spend that 8 bytes to hold on to Calendar if PlainTime really need to use it for some good reason, but right now it is there for nothing and that is a pure waste.

@ptomato
Copy link
Collaborator

ptomato commented Sep 2, 2021

Meeting, Sept. 2: We would like to remove the calendar property from PlainTime, but only if it would be possible to add it in the future without breaking the web. Previously we had determined that this would break the web because you could do plainDateTime.with(plainTime) or plainDateTime.with(plainTime.fields()) and that would start throwing when a calendar property was added. But both of these are no longer possible. We'll continue to investigate if there are other cases that would break.

@ptomato
Copy link
Collaborator

ptomato commented Dec 8, 2022

This will be closed by the change we will make in #1808.

ptomato added a commit that referenced this issue Jan 19, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Jan 21, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Feb 16, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Feb 18, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Feb 27, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Mar 6, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
justingrant pushed a commit to justingrant/proposal-temporal that referenced this issue Mar 11, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: tc39#1808
Closes: tc39#1588
ptomato added a commit that referenced this issue Mar 13, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalLikeObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
ptomato added a commit that referenced this issue Apr 7, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalLikeObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588
@ptomato ptomato closed this as completed in 61fbb40 Apr 7, 2023
justingrant pushed a commit that referenced this issue Apr 20, 2023
PlainTime now has no calendar. Removes the getter and the internal slot.
Calendar annotations are now ignored in ParseTemporalTimeString.

RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is
renamed to RejectTemporalLikeObject.

ToTemporalCalendar rejects PlainTime explicitly instead of looking for its
`calendar` property or treating it as a plain object calendar.

See: #1808
Closes: #1588

UPSTREAM_COMMIT=61fbb401df7463e435b8e6f932de123fd5bc0c2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

5 participants