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

Clarify when entry is added to performance entry buffer #82

Open
igrigorik opened this Issue Nov 30, 2016 · 8 comments

Comments

Projects
None yet
5 participants
@igrigorik
Member

igrigorik commented Nov 30, 2016

Discussion: https://www.w3.org/2016/11/30-webperf-minutes.html

Background: #55. In #79 we added a note to flag the implementation differences across browsers. Ideally, we should aim to converge on a consistent implementation:

  1. It would be great to get some interop tests to identify where/how existing implementations differ on whether the entry is made available before or after the load event.
  2. Real-world telemetry on how often developers rely on querying entry from within onload would, also, be very helpful.
@JosephPecoraro

This comment has been minimized.

Show comment
Hide comment
@JosephPecoraro

JosephPecoraro Feb 7, 2017

Has there been any update on this?

All of the current web-platform-tests seem to require the PerformanceEntry be added before the Resource's load event. However, I'm guessing they existed before PerformanceObserver; which seems to be the major reason to push for allowing entries to be added post-onload.

I was also unable to find a list of interop issues. #55 (comment) pointed to a test file that no longer exists.

Has there been any update on this?

All of the current web-platform-tests seem to require the PerformanceEntry be added before the Resource's load event. However, I'm guessing they existed before PerformanceObserver; which seems to be the major reason to push for allowing entries to be added post-onload.

I was also unable to find a list of interop issues. #55 (comment) pointed to a test file that no longer exists.

@skjindal93

This comment has been minimized.

Show comment
Hide comment
@skjindal93

skjindal93 Oct 15, 2017

@igrigorik I wrote a script to test when an entry is made available in resource timing buffer. I tested it on Chrome and Firefox Nightly using PerformanceObserver.
The PerformanceResourceEntry always gets added to Resource Timing Buffer just before the resource's load event.

I can also help in writing interop tests for this

skjindal93 commented Oct 15, 2017

@igrigorik I wrote a script to test when an entry is made available in resource timing buffer. I tested it on Chrome and Firefox Nightly using PerformanceObserver.
The PerformanceResourceEntry always gets added to Resource Timing Buffer just before the resource's load event.

I can also help in writing interop tests for this

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Oct 27, 2017

Member

The PerformanceResourceEntry always gets added to Resource Timing Buffer before the resource's load event.

We may change that soon in Chrome. Or, at least, there are discussions for pushing delivery of entries towards idle time, which may change what you're seeing.. and such change is compliant to what we specced for PO. /cc @tdresser @spanicker

  • For PO, I don't think we want to make any promises with respect to onload.
  • I'm still of the mind that we shouldn't make before-onload promise for perf timeline either, but this is where @toddreifsteck disagreed before (#55 (comment)).
    • We don't have real-world telemetry on if/whether querying perf timeline within onload is a thing.

Some tests for what the implementations do today, specifically with respect to when entries become available in perf timeline (independent of PO) is the right place to start, I think.. @skjindal93 if you're willing to wire up WPT tests for that, that would be super helpful. If we find that all implementations deliver before onload, we might as well lock that in.

Member

igrigorik commented Oct 27, 2017

The PerformanceResourceEntry always gets added to Resource Timing Buffer before the resource's load event.

We may change that soon in Chrome. Or, at least, there are discussions for pushing delivery of entries towards idle time, which may change what you're seeing.. and such change is compliant to what we specced for PO. /cc @tdresser @spanicker

  • For PO, I don't think we want to make any promises with respect to onload.
  • I'm still of the mind that we shouldn't make before-onload promise for perf timeline either, but this is where @toddreifsteck disagreed before (#55 (comment)).
    • We don't have real-world telemetry on if/whether querying perf timeline within onload is a thing.

Some tests for what the implementations do today, specifically with respect to when entries become available in perf timeline (independent of PO) is the right place to start, I think.. @skjindal93 if you're willing to wire up WPT tests for that, that would be super helpful. If we find that all implementations deliver before onload, we might as well lock that in.

@npm1

This comment has been minimized.

Show comment
Hide comment
@npm1

npm1 Feb 8, 2018

Contributor

I don't think that PerformanceObserver means we should stop adding entries before load. The only significant work from the PerformanceObserver is the callback, and when queueing an entry we only queue a task for that callback, not just run it immediately. What's important is that there is no guarantee that the task will be run before onload, since it is a low priority task.

Contributor

npm1 commented Feb 8, 2018

I don't think that PerformanceObserver means we should stop adding entries before load. The only significant work from the PerformanceObserver is the callback, and when queueing an entry we only queue a task for that callback, not just run it immediately. What's important is that there is no guarantee that the task will be run before onload, since it is a low priority task.

@yoavweiss

This comment has been minimized.

Show comment
Hide comment
@yoavweiss

yoavweiss Jun 18, 2018

Contributor

@npm - so if I understand you correctly, we could have a guaranty that entries are added to the buffer before onload, but not a guaranty that PO will fire before it. Is that correct?

@igrigorik & @toddreifsteck - Assuming that the above is correct, would you be OK with modifying the spec to say that entries MUST be added to the queue before the onload event, but PO tasks may actually fire afterwards?

Contributor

yoavweiss commented Jun 18, 2018

@npm - so if I understand you correctly, we could have a guaranty that entries are added to the buffer before onload, but not a guaranty that PO will fire before it. Is that correct?

@igrigorik & @toddreifsteck - Assuming that the above is correct, would you be OK with modifying the spec to say that entries MUST be added to the queue before the onload event, but PO tasks may actually fire afterwards?

@yoavweiss

This comment has been minimized.

Show comment
Hide comment
@yoavweiss

yoavweiss Jun 18, 2018

Contributor

Note that @npm1 already wrote tests for this, and it seems like browsers are currently consistent about adding entries before onload.

Contributor

yoavweiss commented Jun 18, 2018

Note that @npm1 already wrote tests for this, and it seems like browsers are currently consistent about adding entries before onload.

@npm1

This comment has been minimized.

Show comment
Hide comment
@npm1

npm1 Jun 18, 2018

Contributor

If I understand the comment in [1] correctly, WebKit decided to change their behavior to add entries to the performance buffer asynchronously.

[1] #141 (comment)

Contributor

npm1 commented Jun 18, 2018

If I understand the comment in [1] correctly, WebKit decided to change their behavior to add entries to the performance buffer asynchronously.

[1] #141 (comment)

@yoavweiss

This comment has been minimized.

Show comment
Hide comment
@yoavweiss

yoavweiss Jun 18, 2018

Contributor

@npm1 - yeah, your resources_added_by_onload test no longer seem to pass there (but with a couple of timeouts, so it doesn't seem like onload is losing the race every single time).

In any case, I think we need to separate three cases:

  • When resource entries are added to the timeline buffer - AFAIU, they can be synchronously added in all implementations.
  • When PO callback fires - could be async in all implementations.
  • When resourcetimingbufferfull fires - we need to create a secondary buffer if we want to make it async. (I added a long comment on the issue, and GH seems to have dropped it to the floor :/)
Contributor

yoavweiss commented Jun 18, 2018

@npm1 - yeah, your resources_added_by_onload test no longer seem to pass there (but with a couple of timeouts, so it doesn't seem like onload is losing the race every single time).

In any case, I think we need to separate three cases:

  • When resource entries are added to the timeline buffer - AFAIU, they can be synchronously added in all implementations.
  • When PO callback fires - could be async in all implementations.
  • When resourcetimingbufferfull fires - we need to create a secondary buffer if we want to make it async. (I added a long comment on the issue, and GH seems to have dropped it to the floor :/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment