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

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

Open
Elchi3 opened this issue Apr 2, 2024 · 20 comments · May be fixed by #274
Open

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

Elchi3 opened this issue Apr 2, 2024 · 20 comments · May be fixed by #274

Comments

@Elchi3
Copy link

Elchi3 commented Apr 2, 2024

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
Copy link
Contributor

kenchris commented Apr 2, 2024

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
Copy link
Contributor

arskama commented Apr 2, 2024

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
Copy link
Author

Elchi3 commented Apr 2, 2024

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
Copy link
Contributor

arskama commented Apr 2, 2024

@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
Copy link
Author

Elchi3 commented Apr 2, 2024

I think you want EpochTimeStamp then? See w3c/hr-time#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
Copy link
Contributor

arskama commented Apr 2, 2024

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
Copy link
Author

Elchi3 commented Apr 2, 2024

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
Copy link
Contributor

kenchris commented Apr 3, 2024

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

@kenchris
Copy link
Contributor

kenchris commented Apr 3, 2024

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

  • Changing the type to reflect it being epoch based
  • Moving to a time origin relative time

What do you recommend Yoav?

@Elchi3
Copy link
Author

Elchi3 commented Apr 3, 2024

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
Copy link
Contributor

kenchris commented Apr 3, 2024

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
Copy link
Contributor

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
Copy link
Contributor

kenchris commented Apr 3, 2024

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
Copy link
Contributor

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

@kenchris
Copy link
Contributor

kenchris commented Apr 3, 2024

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

@arskama
Copy link
Contributor

arskama commented Apr 24, 2024

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

@arskama
Copy link
Contributor

arskama commented Apr 24, 2024

closing spec issue, now that we have an implementation bug

@arskama arskama closed this as completed Apr 24, 2024
@rakuco
Copy link
Member

rakuco commented Apr 29, 2024

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 rakuco reopened this Apr 29, 2024
@rakuco
Copy link
Member

rakuco commented Apr 29, 2024

(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
Copy link
Contributor

^^ @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?

arskama added a commit to arskama/compute-pressure that referenced this issue May 30, 2024
arskama added a commit to arskama/compute-pressure that referenced this issue May 30, 2024
@arskama arskama linked a pull request May 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants