SE-0329 (Third review): Clock, Instant, and Duration

Oh, it looks like the implementation does store the value as a 128-bit number of attoseconds. At least in its current form. I thought the comments above indicated that the attoseconds were normalized to 1..<1e18, but maybe that's not the case.

A couple of things:

  1. I'm not sure about DurationProtocol.

    I think that it is going to be quite difficult to use a generic Clock unless you constrain Clock.Instant.Duration == Duration, which is a bit of a mouthful. If you don't add that constraint - what can you actually do? You can get instants from the clock, and get the abstract duration between them, and add and scale those durations, but you can't actually talk about those durations in units of time.

    That's because the proposal is deliberately trying to separate the concept of durations from time - so that it can refer to things such as GPU frames or arbitrary "steps". I think there's a danger of trying to be too generic, and am reminded of Collection.IndexDistance (RIP), which we had to remove and replace with Int, because otherwise generic collections were just too difficult for mere mortals to use.

    Generics are great for building features and algorithms in terms of abstract concepts, but it is good for those abstractions to cap out at some point. I don't feel this strikes the right balance WRT usability of generic clocks.

    The section immediately after DurationProtocol talks about the concrete Duration type, and begins:

    Meaningful durations can always be expressed in terms of nanoseconds plus a number of seconds, either a duration before a reference point or after

    So are durations such as GPU frames not "meaningful"? Perhaps it's poorly worded, but I think it's more of a Freudian slip.

  2. I still don't love the name InstantProtocol.

    I wonder - if we were to make Clock more like Collection, would we even need an InstantProtocol? After all, Collection doesn't have an IndexProtocol - instead, a collection's indexes are basically opaque values, and all operations on them are performed via the collection you got them from.

    There is a strong parallel here. Instants (in their plain generic form) are basically opaque values, and their meaning is that you can calculate how long it will take some specific clock to arrive at that instant, or how long it has been since some specific clock passed that instant. In the fully general case, I think you do need to consult the clock to traverse the timeline, and that this could handle even abstract situations such stepping through a simulation log, without the extra protocol.

  3. Duration API

    I'm also not a huge fan of the split seconds/nanoseconds properties, or really any of the alternatives suggested so far. If I consider the API I'd like to use, I think I'd want to have a Duration value (which is essentially opaque), and to get its value as a floating-point/decimal/integer number of my chosen range and precision. So I could write:

    let duration = Duration.nanoseconds(1234567000)
    
    duration.attoseconds(as: Int128.self)  // 1234567000000....
    duration.attoseconds(as: Double.self)  // 1.234567e+18 or closest Double
    
    duration.milliseconds(as: Decimal.self) // 1234.567ms
    
    duration.seconds(as: Double.self) // 1.234567s, or closest Double
    duration.seconds(as: Int.self)    // 1s
    

    (This may even be a case where a generic return type is warranted, so the storage type can be inferred from the call-site)

    I see this as being similar to the Angle type I proposed for swift-numerics (which SwiftUI recently added) - basically, when I'm providing a duration to an API, I want to specify it as some number of units, as is most convenient for me, and then consumers of that value can extract it as whichever units are most convenient for them. The current design only does half of that.

Okay, please remember that this is the third review, and we are now just reviewing the operations for constructing and deconstructing Duration, and particular the operations for breaking it down into whole seconds, whole milliseconds, whole nanoseconds, second + nanoeseconds, and so on.

5 Likes

Oh sorry, I missed the part about the rest of it being accepted. My mistake!

No problem! You weren't the only one, so I just wanted to get us back on track.

1 Like

Per the discussion in this review, the authors have agreed to change the proposal by making the following changes to the API of Duration:

  • The accessors
    public var secondsPortion: Int64 { get }
    public var nanosecondsPortion: Int64 { get }
    
    have been replaced with the accessor:
    public var components: (seconds: Int64, attoseconds: Int64) { get }
    
  • The following initializer has been added:
    public init(secondsComponent: Int64, attosecondsComponent: Int64)
    

The Core Team has decided that adding initializers to make it easier to interoperate with types such as struct timespec is important but outside of the scope of Evolution review.

This review has been extended until next Monday, the 7th of February, to allow for further discussion of the new proposal. Only the above accessors or closely-related API are on topic for the review. Thank you for your patience with this process.

John McCall
Review Manager.

10 Likes

I'll keep my feedback short:

Great, although as others have pointed out that's a lot of zeros to deal with if one wants to convert to microseconds—nothing that can't be overcome and convenience APIs can be added later if some design is revealed to be clearly the right way to go.

Sensible to allow one to reconstruct Duration from its components. Only one point:

  • Component really isn't adding anything here, in my view: init(seconds:attoseconds:) is perfectly intelligible
8 Likes

I want to add that the Core Team sees the importance of having an Int128 type for the use of APIs like Duration and that we'll be taking that under consideration for the future, but we felt that that was clearly out of scope for this review.

13 Likes

ditto

1 Like

Maybe the reasoning is to not confuse users with a future init(attoseconds: Int128)?

1 Like

What would be the confusion?

Why does the initializer contain component, but the getter does not? This discrepancy only further highlights the confusion possible between “total duration as attoseconds” and “fractional seconds expressed as attoseconds”, especially once var attoseconds: Int128 { get } becomes possible.

Can @Philippe_Hausler or someone from the Core Team speak to why this specific alternative was chosen?

The reason why is that immediately would cause ambiguity with other values passed in for future API potentials. E.g. a tuple of seconds and milliseconds etc. That was rejected because it would prevent future proposals that definitely have potential space for pitching.

To be clear, this is a review, and if you'd like to offer feedback on the proposal, you'll welcome to do so.

@John_McCall, I’m not sure whether your comment was directed towards me. My question is feedback. I suppose I could phrase it as a statement: proposals usually explain why they chose certain alternatives over others, and this revision does not provide any supporting arguments for the choice of (seconds:, attoseconds:). But that seems excessively process-oriented and indirect.

My strong preference remains for var wholeSeconds: Int64 { get }; func fractionalSeconds(in: FractionalSecondsScale) -> Int64. It is unambiguous as to what portion of the duration the sub-second value spans, and it spares clients the opportunity to write bugs of the kind that @benrimmington found and fixed.

I should addend my feedback to state that, for the initializer, the distinction between these two interpretations can be rendered moot—and it would be good to clarify if the proposal authors agree—by accepting all input such that (notionally) self._internal128BitStorageInAttoseconds = seconds * 1e18 + attoseconds; that is, by allowing attoseconds arguments that exceed 1e18.

This has reminded me that I believe the initializer should either be failable or trap. In one interpretation, it would fail/trap if attoseconds exceeds 1e18. In your alternative, it would trap if attoseconds + (seconds * 1e18) overflows.

Int64.max * Int64(1e18) + Int64.max doesn't exceed Int128.max—overflow is not possible.

2 Likes

This is the source of my confusion. As written I would expect the attosecondsComponent argument to be strictly less than 1 second and I would expect the attoseconds argument to be unbounded.

But thats just how I interpret the API before reading the future API docs.

1 Like

IMO the getter already has one. eg. we’re using .components.seconds to access the seconds component, so it’s pretty fine.