w3c / compute-pressure

A web API proposal that provides information about available compute capacity
https://www.w3.org/TR/compute-pressure/
Other
69 stars 10 forks source link

Is PressureRecord.time a timestamp or a time that is relative to timeOrigin? #257

Closed Elchi3 closed 5 months ago

Elchi3 commented 7 months ago

The spec says:

a [[Time]] value of type DOMHighResTimeStamp, which corresponds to the time the data was obtained from the system, relative to the time origin of the global object associated with the PressureObserver instance that generated the notification.

In Chrome Canary I'm getting timestamps, though. (e.g., 1712050826399.263, so maybe it is a Chrome bug?). I was expecting a smaller number similar to when I call performance.now().

Follow up question: If I should be seeing relative time stamps, do they respect performance.timeOrigin so that I could sync times between window and worker contexts?

kenchris commented 7 months ago

Currently this is what the spec says, but we can change to something else if that makes more sense.

a [[Time]] value of type DOMHighResTimeStamp, which corresponds to the time the data was obtained from the system, relative to the time origin of the global object associated with the PressureObserver instance that generated the notification.

arskama commented 7 months ago

About the implementation (on Chromium):

The difference is that PerformanceObserver is getting the time from "TickClocks class", which is for iframe (e.g), relative to the time of creation of the frame, threfore the number is smaller in your experience.

In ComputePressure it s the current time. (Which might be affected by the system).

Unless mistaken, they are both DOMHighResTimeStamp.

It makes no difference to us if the observer is run from a worker or a frame since the sample time is fetch from the same backend.

I need to test out what would be the effect if using TickClocks instead of Time class in Chromium. Actually it would prevent the possible issue from system adjustment.

Elchi3 commented 7 months ago

I guess the main thing I want to understand (for the documentation) is:

What is PressureRecord.time relative to? 1) Is it relative to the Unix Epoch (January 1, 1970, UTC)? Chrome returns "1712064622387.064" so that seems like a Unix Epoch relative timestamp to me. 2) Or is it meant to be relative to performance.timeOrigin, i.e the time when navigation has started (window context) or the time when the worker is run (worker context).

If the answer is 1), then I think the spec text is confusing as it talks about being "relative to the time origin" If the answer is 2), then I wonder why Chrome returns such a large number that looks like a Unix Epoch relative timestamp.

arskama commented 7 months ago

@Elchi3, you are correct, the implementation is relative to the Unix Epoch (January 1, 1970, UTC).

The question is now, what would make more sense? To use Unix Epoch, or another monotonic clock implementation.

I don't think that 2) as defined in performance.timeOrigin, makes sense for Compute Pressure, since it would be useful to have the same origin for workers and window, without manipulation of the timestamp.

@Elchi3, if I understood, your concern is more about the wording of the specification. If we keep the Unix Epoch timestamp, what changes would the spec require?

Elchi3 commented 7 months ago

I think you want EpochTimeStamp then? See https://github.com/w3c/hr-time/issues/149 (Nope, DOMHighResTimeStamp can be relative to the Unix Epoch so that's alright.)

@Elchi3, if I understood, your concern is more about the wording of the specification. If we keep the Unix Epoch timestamp, what changes would the spec require?

Maybe something like this? (I'm no spec editor at all)

a [[Time]] value of type DOMHighResTimeStamp, which corresponds to the time the data was obtained from the system, relative to the Unix Epoch.

arskama commented 7 months ago

Yes, Thanks for pointing out the discussion! As you mentioned, Unix Epoch is also valid with DOMHighResTimeStamp. @kenchris let s look at a better wording together.

Elchi3 commented 7 months ago

The question is now, what would make more sense? To use Unix Epoch, or another monotonic clock implementation.

I don't think that 2) as defined in performance.timeOrigin, makes sense for Compute Pressure, since it would be useful to have the same origin for workers and window, without manipulation of the timestamp.

fwiw, I don't know the answer to this. My feeling is that in the Performance APIs, developers are used to working with document-creation-relative-time and not epoch-relative-time.

I guess currently it would be mixed time information if you use compute pressure with other time markers. For example:

function startVideoCall
  performance.mark("video-call-started");
  observer.observe("cpu"); 

function endVideoCall
  performance.mark("video-call-finished");
  observer.disconnect()

// time video-call-started: 5000.23
// time pressure state nominal: 1712050826399.263
// time pressure state fair: 1733350826399.263
// time pressure state nominal: 1788850826399.263
// time video-call-finished: 30000.23 

So if we had the same time origins, the advantage would be that the compute pressures could all be lined up in a timeline with other performance-related events.

// time video-call-started: 5000.23
// time pressure state nominal: 8000.57
// time pressure state fair: 20000.67
// time pressure state nominal: 25000.44
// time video-call-finished: 30000.23 
kenchris commented 7 months ago

I also assume that if you want epoch you can take the relative number and add it to the time origin?

kenchris commented 7 months ago

Adding @yoavweiss here, as he should be able to give us advice on the best way forward:

What do you recommend Yoav?

Elchi3 commented 7 months ago

I also assume that if you want epoch you can take the relative number and add it to the time origin?

Yes!

performance.timeOrigin + computePressureRecord.time = epochtimestamp (if computePressureRecord.time would be time origin relative)

And for synchronization between window and worker, the timeOrigin property helps you to translate as well. See this example: https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin#synchronizing_time_between_contexts

kenchris commented 7 months ago

So this basically means that the spec is in good shape, but we have an implementation bug. Might make sense to add some examples to the spec or mdn though :-)

yoavweiss commented 7 months ago

I think it'd be better for y'all to more strictly define what timestamp y'all want to get. The current relative timestamp seems like a reasonable choice for what you're after.

kenchris commented 7 months ago

so something along the lines of?

a [[Time]] value of type DOMHighResTimeStamp, representing the [=current relative timestamp=] of the [=relevant settings object=] associated with the PressureObserver instance that generated the notification.

yoavweiss commented 7 months ago

That sounds about right, assuming that the PressureObserver has an associated environment settings object.

kenchris commented 7 months ago

Indeed, that would be the relevant settings object of that instance. Tried to update the text above :-)

arskama commented 7 months ago

Specs are clear, implementation is not. It is now tracked here: crbug

arskama commented 7 months ago

closing spec issue, now that we have an implementation bug

rakuco commented 7 months ago

I'm reopening the spec issue because the wording should still be adjusted here:

I think what needs to happen is something similar to how Generic Sensors handles timestamps (https://w3c.github.io/sensors/#update-latest-reading):

rakuco commented 7 months ago

(Using the terms from the HR-time spec the right way is always confusing to me, so @yoavweiss is very welcome to correct the above, which would also lead to a fix or two in the Generic Sensors spec :-)

yoavweiss commented 6 months ago

^^ @noamr

I think we want the timestamp to be relative to the time origin (so maybe fix the Chromium implementation). Can't it just represent the current monotonic time of the PressureRecord's relevant settings object?