-
Notifications
You must be signed in to change notification settings - Fork 15
Avoid nonstandard use of names for types in wit-0.3.0-draft #79
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Philip Chimento <[email protected]>
ec27938
to
b7d7920
Compare
wit-0.3.0-draft/system-clock.wit
Outdated
resolution: func() -> instant; | ||
// NOTE: This return value doesn't represent an exact time, so maybe is not | ||
// a correct use of the Instant type. Would it make sense to have a | ||
// system-clock::duration analogous to monotonic-clock::duration? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree there should be a duration type in system-clock, can you add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
It's a little silly to use a seconds+nanoseconds struct for this given that a u64 of nanoseconds can represent durations up to 585 years and system clock resolution is normally going to be at most 16 milliseconds. Even a u32 of nanoseconds (~4 seconds) would probably suffice. But system-clock:duration
is the most conceptually correct type and this is not going to be on the critical path anywhere, so it shouldn't really matter. And it does match clock_getres
.
Thank you for making this change to align us with the Temporal proposal. One aspect I am a little skeptical of is the change of Could the value given by |
I'm OK with overloading "instant" if that's the best option but would prefer not to given an acceptable alternative. Also it would be a little weird to have system clock "instant"s be a struct but have monotonic clock "instant"s be a u64. Unfortunately there's not a lot of good prior art for the return value of a monotonic clock; other languages mostly just return a Names I'm aware of for things vaguely in this space:
I don't like "instant" because it's overloaded; I don't like names with "time" because these aren't really times per se, just values which can be diffed to get a duration; I don't like "tick" because the length of a "tick" is usually system-dependent. I suggested "clock point" because this represents just a point on a particular clock, nothing more or less. But these are just my personal opinions. Other than "clock point", I think my favorite name would be "mark", following Kotlin: I think the name reasonably suggests that you can take the difference between two marks but that they don't have any meanings on their own. |
Thanks for looking at the field of choices there. I would be fine with using anything we can reasonably justify as following at least some other existing use of a term, rather than inventing our own. On aesthetics I also like kotlin's "mark" for this use. |
OK, switched to "mark" for the moment, pending anyone having a better idea. "mark" does also have some similarity to the web's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "mark" in place of the monotonic clock's "instant", and "instant" in place of the wall clock's "datetime" seem fine to me.
wit-0.3.0-draft/system-clock.wit
Outdated
@since(version = 0.3.0) | ||
record datetime { | ||
record instant { | ||
seconds: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pre-existing, however while you're here, could you also change seconds
from u64
to s64
? That better reflects the operating system APIs that it will be commonly implemented on, and supports timess from before 1970.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. What about for durations? Is it intentional that durations cannot be negative? (See also tc39/proposal-temporal#558.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that making durations potentially negative for the monotonic clock is somewhat tricky unless mark
is also changed to use s64
, because the difference between the largest u64
and 0
is not representable as an s64
and hence a s64
duration
would not be able to represent all possible differences between two mark
s.
(In practice this will never come up because no one is going to observe monotonic clock values which are 294 years apart, so I guess implementations could trap or something.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, wasi-clocks has a much smaller surface area than Temporal
, and it doesn't have any uses for negative durations. However, I'd be open to using a signed type. I think we could go either way. If it's signed, we just need to make sure we document that things like resolution
don't return negative values, and document what subscribe-durection
does given a negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly I ask because the readme has an example of a language creating a duration by subtracting two values from the clock, and it's not clear what the behavior would be if they'd reversed the order of those values. If the answer is "that's on the language to figure out" that's fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current design is that that's on the language to figure out.
/// time. The name "wall" makes an analogy to a "clock on the wall", which | ||
/// is not necessarily monotonic as it may be reset. | ||
/// WASI System Clock is a clock API intended to let users query the current | ||
/// time. The clock is not necessarily monotonic as it may be reset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We originally avoided the term "system clock", but I think it's ok to use now.
Background: we wanted WASI to deemphasize the idea of "the system", because that may suggest a level of monolithicity that we don't want to require. However, if "wall clock" isn't suitable, I'm guessing we won't find any other names that have the desired meaning without risk of being mistaken. So unless we think of some brilliant alternative name, I think "system clock" is the least-problematic option.
seconds: s64, | ||
nanoseconds: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct IMO, but might be worth a note that nanoseconds are unsigned because even negative seconds values are still conceptually "positive" exact times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was just thinking of this as, the way you'd represent one nanosecond before the epoch would be { -1 seconds, +999_999_999 nanoseconds }.
I'm not sure what you mean by 'conceptually "positive" exact times'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think of it in the same way as you. What I meant is Dec 31 1969 isn't "negative" in any way other than being before the epoch, which is arbitrary.
The (wrong) alternative interpretation would be to think of { seconds, nanoseconds } as a "duration" before the epoch, e.g. -1.999999999 nanoseconds. That would result in Dec 31 1969 23:59:58.000000001.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note which hopefully clarifies:
Note that even if the seconds field is negative, incrementing nanoseconds always represents moving forwards in time. For example,
{ -1 seconds, 999_999_999 nanoseconds }
represents the instant one nanosecond before the epoch.
wit-0.3.0-draft/system-clock.wit
Outdated
@@ -47,6 +47,7 @@ interface system-clock { | |||
|
|||
/// Query the resolution of the clock. | |||
/// | |||
/// The seconds field of the output is always positive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The seconds field of the output is always positive. | |
/// The seconds field of the output is always positive or zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. Actually this note is unnecessary unless we're making durations signed, so I'll just remove it for now but if it goes back I'll fix this.
Any more to do here? |
@yoshuawuyts: Should this target 0.2.5 using |
This is the renaming part of #71 by @ptomato applied to the new wit-0.3.0-draft directory. (Edit: as well as some other changes prompted by discussion below.)
As discussed in #69 (comment), the use of the terms "wall clock" and "instant" here is very confusing: in JavaScript, Java, Go, and etc, an "instant" is a particular absolute point in time, and "wall clock" time means a local time without associated timezone and therefore not referring to a particular point in time. (More on this topic in the excellent Temporal docs.)
By contrast, here "instant" does not refer to a particular absolute point in time, and "wall clock" time is used to mean a particular absolute point in time. I'm hoping this can be fixed to use the more conventional names.
(There's also a couple of
// NOTE
comments about weird types.)Note also that #71 contains proposed changes to the unstable timezone interface. I'd be very happy to rebase those onto wit-0.3.0-draft as well, if you'd like, though note that per the final paragraph of this comment the interface probably needs at least one additional method than is currently included in that PR.