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

Timestamps should be specified in an interoperative way #62

Closed
smfr opened this issue Mar 5, 2020 · 22 comments · Fixed by #108
Closed

Timestamps should be specified in an interoperative way #62

smfr opened this issue Mar 5, 2020 · 22 comments · Fixed by #108
Assignees

Comments

@smfr
Copy link

smfr commented Mar 5, 2020

Instead of:

the timestamp should be the latest timestamp the user agent is able to note in this pipeline (best effort). Typically the time at which the frame is submitted to the OS for display is recommended for this API.

the timestamps should be the time at the end of the "update the rendering" steps, namely the same timestamp used for long task reporting, in section 12.1 of https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

Trying to guess when the OS is going to present the frame is fraught with peril and adds no value.

@npm1
Copy link
Contributor

npm1 commented Mar 5, 2020

Sounds good to me! Actually I think the order of paint timing call and update the rendering is incorrect. That is, the step "For each fully active Document in docs, update the rendering or user interface of that Document and its browsing context to reflect the current state." would need to go before the paint timing step. And then we can compute the timestamp right there, as there is now a step in between regarding requestIdleCallback which should not affect the computations. I'll need to update the order in HTML for this to work.

@rniwa
Copy link

rniwa commented Mar 5, 2020

Well, step 10.13 already involves mark painting time so we just need to get rid of this informal note.

I don't think we should use step 12.1 since that exposes the precise painting time, which is dangerous for timing attacks.

@npm1
Copy link
Contributor

npm1 commented Mar 5, 2020

You're saying that the timestamp should be the now that is passed on to the other callbacks? That one does not seem to include anything about the "update the rendering" step though.

@rniwa
Copy link

rniwa commented Mar 6, 2020

You're saying that the timestamp should be the now that is passed on to the other callbacks? That one does not seem to include anything about the "update the rendering" step though.

I'm not saying that. In step 10.13, updating the rendering invokes mark painting time. I'm saying that we should use the current time at that point.

Now, there are a few editorial bugs here.

First, mark painting time takes an input timestamp, that's not being passed to the algorithm in the event loop processing model. We need to either specify the timestamp in the event loop processing model, or use the current time in the mark painting time algorithm itself.

Second, the event loop processing model says that mark painting time is invoked on each document but the algorithm doesn't take a document as an argument. Once we've fixed it so that mark painting time takes a document as an argument, then we need to refine the definitions of the first paint and the first contentful paint so that those timings are well defined for a given document.

Finally, we need to make mark painting time pass this document to reporting paint timing, and the report paint timing algorithm needs to create PerformancePaintTiming object with that document.

@npm1
Copy link
Contributor

npm1 commented Mar 6, 2020

Thanks for pointing out the editorial issues, I also noticed them when going over the steps. Will fix soon.

npm1 added a commit that referenced this issue Mar 13, 2020
Provide the realm when constructing the PerformancePaintTiming object. This way, the queue algorithm will use the correct relevant global object. For #62
noamr pushed a commit that referenced this issue Mar 19, 2020
Provide the realm when constructing the PerformancePaintTiming object. This way, the queue algorithm will use the correct relevant global object. For #62
@yoavweiss
Copy link
Contributor

Was this fixed by #67 and/or #69?

@rniwa
Copy link

rniwa commented Mar 25, 2020

No, that text is still in the note:

NOTE: The rendering pipeline is very complex, and the timestamp should be the latest timestamp the user agent is able to note in this pipeline (best effort). Typically the time at which the frame is submitted to the OS for display is recommended for this API.

This note should be removed now that mark the paint timing is now called at the very precise timing in update the reddening step. There is no leeway left for UA to report a best-effort time, and that's a good thing both for security & interoperability.

@noamr
Copy link
Contributor

noamr commented Apr 19, 2024

Coming back to this issue from 4 years ago, I think I have a proposal that would work for both tihs, #104, and w3c/largest-contentful-paint#104.

The idea is that the paint timestamp would be the rAF timestamp of the next frame. So if the rendering opportunity that produces the relevant paint creates some sort of presentation delay, it would be reflected in paint-timing/LCP in an way that's already speified and observable. This would also mitigate anything that has to do with exposing render time of cross-origin images as it's an already exposed timestamp.

@smfr @sefeng211 @mmocny @yoavweiss WDYT?

@mmocny
Copy link

mmocny commented Apr 19, 2024

I like it.


Marking timestamps aside, there is also the decision of which rAF to mark. I don't know if we need to clarify further, or if its already specified sufficiently in paint timing, but it should be the rAF frame time of the animation frame that follows after the content was presented.

It might only be best-effort or somewhat UA specific, but there are situations (i.e. image decode) where I think a first paint merely requests decode to start, and then there is a future paint (perhaps many rAF's afterwards), that actually paints the decoded image.

@anniesullie
Copy link

anniesullie commented Apr 19, 2024

@bdekoz too (see above comment)

@smfr
Copy link
Author

smfr commented Apr 19, 2024

How do you know what the rAF timestamp for the next frame will be? That frame might be delayed.

@noamr
Copy link
Contributor

noamr commented Apr 19, 2024

How do you know what the rAF timestamp for the next frame will be? That frame might be delayed.

We wait to queue the entry and set its timing until that happens, or something like hiding the document happens.

@mstange
Copy link

mstange commented May 17, 2024

So if nothing triggers a paint for a while, this would mean that the timestamp can be much delayed, no?

I don't think this proposal adds a lot of value over using what Simon proposed, i.e. getting the timestamp at the end of the "update the rendering" steps. In Firefox, waiting for the next frame would add an arbitrary delay to the reported timestamp. The delay would be unrelated to the time when the compositor thread is done processing the current frame: If compositing is slow, then we do the "update the rendering" steps for the next frame before compositing has finished (on a different thread) for the current frame, because we allow compositing to fall behind by up to two frames. And if compositing is fast, but the page isn't triggering any more paints, then the next frame would happen long after compositing finishes for the current frame.

I would prefer missing the compositing part entirely over adding an arbitrary delay that is unrelated to the time that compositing takes.

@noamr
Copy link
Contributor

noamr commented Jul 19, 2024

Summary from WebPerfWG call: the general direction is to align Chromium FCP time with the spec. Chromium still intends to report the current number as presentationTime.
Before doing so we will produce some data as to the difference between these numbers in the wild.

@mmocny
Copy link

mmocny commented Jul 22, 2024

I like the general idea of exposing both paint timings for easier comparison and better interop. However:

  • The specced paint time is currently only exposed via startTime (or as renderTime or duration in other "paint timings")
  • Changing this value in Chromium would have a significant effect on developers, even if the current value is still exposed via a new presentationTime

In the past, we discussed exposing both both timings explicitly as new properties:

  • paintTime and
  • presentationTime (optional)

Then, defining startTime (and/or duration where applicable) as based on max(paintTime, presentationTime).

This type of approach already happens for LCP via startTime = max(element.loadTime, element.renderTime, fcp.startTime) or something like that.

This would have no breaking change for Chromium, would improve interop, and enable more direct comparison between implementations (where desired).

(There was a thread about the above in the past, I guess somewhere else...)

@noamr
Copy link
Contributor

noamr commented Jul 22, 2024

I like the general idea of exposing both paint timings for easier comparison and better interop. However:

  • The specced paint time is currently only exposed via startTime (or as renderTime or duration in other "paint timings")
  • Changing this value in Chromium would have a significant effect on developers, even if the current value is still exposed via a new presentationTime

In the past, we discussed exposing both both timings explicitly as new properties:

  • paintTime and
  • presentationTime (optional)

Then, defining startTime (and/or duration where applicable) as based on max(paintTime, presentationTime).

This type of approach already happens for LCP via startTime = max(element.loadTime, element.renderTime, fcp.startTime) or something like that.

This would have no breaking change for Chromium, would improve interop, and enable more direct comparison between implementations (where desired).

(There was a thread about the above in the past, I guess somewhere else...)

This is fine with me

@smfr how do you feel about this? It basically means that WebKit exposes paintTime which is an alias for startTime, and the spec says that there's an optional presentationTime and that startTime maps to it if it is provided.

@mstange
Copy link

mstange commented Nov 7, 2024

and the spec says that there's an optional presentationTime and that startTime maps to it if it is provided.

This sounds fine to me from the Firefox side.

@noamr
Copy link
Contributor

noamr commented Nov 7, 2024

and the spec says that there's an optional presentationTime and that startTime maps to it if it is provided.

This sounds fine to me from the Firefox side.

To clarify the proposal in detail, as discussed at WebPerfWG:

  • paintTime would be the current time at end of "update the rendering steps", as measured here.
  • presentationTime is an optional implementation-defined time after that, roughly corresponding to VSync, extra-coarsened.
  • These two attributes would be present in the following entry types: first-paint, first-contentful-paint, element, largest-contentful-paint, event, long-animation-frame.
  • The following attributes would return presentationTime if exists, and paintTime otherwise: first-paint and first-contentful-paint's startTime, element and largest-contenful-paint's renderTime, event's end time.

noamr added a commit that referenced this issue Nov 12, 2024
- `paintTime` (the end of "update the rendering phase")
- `presentationTime` (an optional implementation-defined timestamp)

`startTime` returns `presentationTime`, or `paintTime` if `presentationTime`
is not implemented.

Currently this `PaintTimingMixin` is only included in `PerformancePaintTiming`,
but it will be included in `ElementTiming`, `LargestContentfulPaint`,
`PerformanceEventTiming`, and `PerformanceLongAnimationFrameTiming` as a follow up.

This is based on a resolution at the web performance working group.
Minutes: https://docs.google.com/document/d/1ZWAUJZBJUvSUyShvKXNEU-cuCe47jpgbR69ckWZUTfI/edit?tab=t.0#heading=h.kb3idfbysfg7

Closes #62
@noamr noamr closed this as completed in 5260924 Nov 12, 2024
noamr added a commit to noamr/element-timing that referenced this issue Nov 13, 2024
Given w3c/paint-timing#62, we now expose paint timestamps as part
of PaintTimingMixin.

This PR extends PerformanceElementTiming to include that mixin and propagates
the right calls.
noamr added a commit to w3c/largest-contentful-paint that referenced this issue Nov 13, 2024
Instead of initializing `renderTime`, take it from the paint timing info
as propagated from paint timing.

See w3c/paint-timing#62 and wicg/element-timing/#81
@yoavweiss yoavweiss reopened this Nov 18, 2024
@yoavweiss
Copy link
Contributor

Let's keep this open for now. I'd love to get confirmation from @smfr that the solution is one he's happy with.

@noamr
Copy link
Contributor

noamr commented Nov 18, 2024

Let's keep this open for now. I'd love to get confirmation from @smfr that the solution is one he's happy with.

Sure, sounds good!

@smfr
Copy link
Author

smfr commented Nov 18, 2024

Seems OK.

@yoavweiss
Copy link
Contributor

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants