Clarify addition of PerformanceResourceTiming

[npm1]

Fixes #95
@tdresser
@igrigorik
@marcoscaceres

---

Preview | Diff

[igrigorik]

We have three outstanding issues related to this logic:

• Clarify resourcetimingbufferfull conditions: Clarify resourcetimingbufferfull conditions #95
• When is resourcetimingbufferfull supposed to fire: When is resourcetimingbufferfull supposed to fire? #141
• setResourceTimingBufferSize should not clear "resource timing buffer full flag" when the buffer has shrunk: setResourceTimingBufferSize should not clear "resource timing buffer full flag" when the buffer has shrunk #145

To ensure that we're considering the full picture, could we address these as a single PR?

cc: @JosephPecoraro @smaug---- @rniwa @toddreifsteck

[npm1]

Sure sounds good, I'll take a look at the other issues and update this PR.

[marcoscaceres]

Drive-by comments 🚙💨

[npm1]

Wait where are the comments? :)

By the way I'm not yet sure how to solve issue 141 which is why I haven't updated the PR yet. Maybe I'll add a generic comment saying that the entry should have been added by onload, and this would be accompanied by tests using onload showing that this is currently the case on the major browsers?

[marcoscaceres]

Sorry, forgot to press “submit review” 🙃

[marcoscaceres]

seems odd that you are defining Performance here? Did you maybe mean to just link to it?

[npm1]

Done

[marcoscaceres]

You might want to check the "fire and event" algo, as it changed recently. At the very least, the "simple" is no longer part of the DOM spec, I think.

[npm1]

I removed the word 'simple' because I didn't see it but I also didn't find any recent changes (< 1year) to the fire event algorithm. Is there anything in particular you were thinking about?

[marcoscaceres]

Apologies, I was recalling in Payment Request we had to formulate the prose for firing the event in a very particular way, but that's because we defined a new event interface. With just using Event, I think your current text is all good.

[npm1]

I've added minimal changes to fix the three issues - please take a look.

[igrigorik]

This is contrary to the conclusion in #82 (comment).

Let's resolve #141 (comment) before we land this.

[npm1]

Steps 19 and 20 do not involve much work at all and there is no reason for them to not run synchonously. I think what we wanted to delay in Chrome was executing the task in step 6 of https://www.w3.org/TR/performance-timeline-2/#dfn-queue-a-performanceentry
This is actually already spec'd as a low priority task, so Chrome is just trying to actually make it low priority by only running it when idle.

[igrigorik]

We did not want to encourage use and assumption of entry being available before onload: if the PO entry is delivered asynchronously, we should apply same logic to queuing them into the buffer. Further, if we ever wanted to include processing time of the resource (e.g. #133), locking RT to be queued before onload event would then clash down the road.

[npm1]

I don't see a problem with having a discrepancy between PO callbacks and entries in buffer - the fact that the callback is async is needed for performance reasons, right? And processing time sounds interesting, but I'm not sure how safe it is to delay pushing the entry by a lot, given that the existing implementations seem to add entries to the buffer ASAP. I'm working on tests to see how browsers behave with several types of resources, but if they all have entries by onload, then I would oppose to specing something that is contrary to that. This is a contentious point so I can remove this 'solution' to issue #141 for now but we should make a decision on this sometime.

[igrigorik]

I'm working on tests to see how browsers behave with several types of resources

Great. If we have this in time, let's review this on the next call. I'd like to get more eyeballs on this before we determine on how to proceed.

[yoavweiss]

However, I think this might also make #133 tricky

Good point! At the same time, if as a result of #137 we'll end up making the entries mutable (similar to PerformanceNavigationTiming, as discussed on the call) that'd be less of an issue.

[igrigorik]

True, but that's also rather odd behavior. In this case we're talking about entries available via global timeline, not PerfObserver. You could make the case that we already surface "incomplete" entries via NavTiming, but this just perpetuates the confusion.

[yoavweiss]

From an analytics provider's perspective, incomplete entries (marked as such, with an event indicating mutation or completion) are better than no entries.

Regardless, this seems like something that all current implementations agree on, so changing this behavior in the future wouldn't be harder than it is right now. (and I suspect it's already not web compatible to change it, but could be proven wrong)

[igrigorik]

Regardless, this seems like something that all current implementations agree on, so changing this behavior in the future wouldn't be harder than it is right now.

Fair enough, I think you have me convinced. Let's get a final ack from wider group on our next call, and we can proceed?

[yoavweiss]

SGTM

[igrigorik]

@JosephPecoraro @rniwa can you please review this to make sure we've addressed #95, #145.

[yoavweiss]

LGTM

[igrigorik]

Let's discuss this in our WG call before merging.

[Soltik]

It seems that the logic described in section 4.4 does not prevent from missing RT events:

1. If resource timing buffer current size is less than resource timing buffer size limit, run the following substeps:
   a. Add new entry to the performance entry buffer.
   b. increase resource timing buffer current size by 1.
2. Otherwise, if the resource timing buffer full flag is false, run the following substeps:
   a. fire a simple event named resourcetimingbufferfull at the Document, with its bubbles attribute initialized to true, and has no default action.
   b. set the resource timing buffer full flag to true.

Algorithm should include saving of current RT entry:

1. If resource timing buffer current size is less than resource timing buffer size limit-1, run the following substeps:
   a. Add new entry to the performance entry buffer.
   b. increase resource timing buffer current size by 1.
2. Otherwise, if the resource timing buffer full flag is false, run the following substeps:
   a. Add new entry to the performance entry buffer.
   b. fire a simple event named resourcetimingbufferfull at the Document, with its bubbles attribute initialized to true, and has no default action.
   c. set the resource timing buffer full flag to true.

Even simplier way:

1.. If resource timing buffer current size is less than resource timing buffer size limit, run the following substeps:

a. Add new entry to the performance entry buffer.
b. increase resource timing buffer current size by 1.
c. If resource timing buffer current size is equal or greater than resource timing buffer size limit and the resource timing buffer full flag is false, run the following substeps:
c1. set the resource timing buffer full flag to true.
c2. fire a simple event named resourcetimingbufferfull at the Document, with its bubbles attribute initialized to true, and has no default action.

And another way:
Not to change the algorythm, but to add current RT entry into arguments of event function, so the developer gets at least one lost entry inside callback.

[yoavweiss]

It seems that the logic described in section 4.4 does not prevent from missing RT events:

If resource timing buffer current size is less than resource timing buffer size limit, run the following substeps:
a. Add new entry to the performance entry buffer.
b. increase resource timing buffer current size by 1.
Otherwise, if the resource timing buffer full flag is false, run the following substeps:
a. fire a simple event named resourcetimingbufferfull at the Document, with its bubbles attribute initialized to true, and has no default action.
b. set the resource timing buffer full flag to true.

That's not what this PR actually says. It actually fixes this by removing the "otherwise" and adding an extra condition instead.
So after this PR lands, the processing would be:

1. If resource timing buffer current size is less than resource timing buffer size limit, run the following substeps:
   1. Add new entry to the performance entry buffer.
   2. Increase resource timing buffer current size by 1.
2. If resource timing buffer current size is greater than or equal to resource timing buffer size limit and the resource timing buffer full flag is false, run the following substeps:
   1. Set the resource timing buffer full flag to true.
   2. Fire an event named resourcetimingbufferfull at the Performance object, with its bubbles attribute initialized to true and with no default action.

Stepping through this, if the entry we're about to add will reach the buffer's capacity, it will be added (as we'll still pass the "less than" condition), and the counter will be incremented.
At that point, the counter will be equal to the max size, so we'll also fall into the second condition, set the flag to true and fire an event indicating that the buffer is now full.

[npm1]

Tim told me that from the discussion in Web Perf WG we would like to land this without the change to require entries to be on the buffer before onload. I didn't quite understand the problem with Safari developer preview, but they said they no longer guarantee entries to be in before onload right @rniwa ? I don't know the reasoning behind this change in Safari but ok. Just want to confirm that I should go ahead and remove the onload part of this PR.

[rniwa]

Yes, in WebKit, we don't resourcetimingbufferfull event until the next tasks, and once scripts had cleared the buffer, we'd add more entries. This would mean that load event for some resources could fire before corresponding resource timing entries are inserted into the buffer.

[npm1]

and once scripts had cleared the buffer, we'd add more entries.

Which entries would you add? If it's the ones added when the resource timing buffer is full, then I think your interpretation of the spec is not correct because the point of having a limit to the buffer size is to limit the amount of memory we spend on storing entries. If you're storing them even when the resource timing buffer is full, then what's the point of having a limit on the size of the buffer? It should also be noted that that implementation makes Safari inconsistent with other browser vendors: Safari will deliver a different amount of resource timing entries than the rest of browsers, which will ignore entries that occur when the buffer is full.

[rniwa]

and once scripts had cleared the buffer, we'd add more entries.
Which entries would you add? If it's the ones added when the resource timing buffer is full, then I think your interpretation of the spec is not correct because the point of having a limit to the buffer size is to limit the amount of memory we spend on storing entries.

Perhaps you misunderstood what I'm saying. What we have is a secondary buffer which is used to queue up entries until resourcetimingbufferfull event is fired. Once the event is fired and the primary buffer exposed to scripts aren't cleared, then we'd simply drop those entires. We don't indefinitely queue up more entries.

If you're storing them even when the resource timing buffer is full, then what's the point of having a limit on the size of the buffer?

The point is to be compatible with other browsers which don't drop entries on floor when the primary buffer is full.

It should also be noted that that implementation makes Safari inconsistent with other browser vendors: Safari will deliver a different amount of resource timing entries than the rest of browsers, which will ignore entries that occur when the buffer is full.

This is absolutely not true. We deliver exactly the same number of resource timing entires as other browsers. We simply delay the dispatching of resourcetimingbufferfull event until the next task. While the event is being delayed, we queue up entries in a secondary buffer.

[npm1]

Ok, that explanation is a bit clearer. I still think it's a bad idea to have very different implementations (although I guess one benefit of this is encouraging developers to use PerformanceObserver instead?). Safari is delaying the resourcetimingbufferfull event and then trying to simulate the entries as if it had been fired as soon as the resource loaded. To me, that's not intuitive at all and may cause subtle bugs.

For now, I'll remove the onload change and ask for another review.

[npm1]

PTAL, reverted onload change.

[igrigorik]

👍 with the new update.

Tagging #82 for reference.

@npm1 thanks for driving the work and discussion on this one!