Fire `resourcetimingbufferfull` asynchronously #163

yoavweiss · 2018-08-30T13:05:45Z

Fixes #141 and #96

This aligns the spec with WebKit's implementation to make sure that resourcetimingbufferfull can fire asynchronously without losing any entries in the process.

Tests are still missing.

@igrigorik @npm1 - can you take a look?

Preview | Diff

yoavweiss · 2018-08-30T13:10:30Z

@JosephPecoraro @rniwa - can you take a look as well?

npm1 · 2018-08-30T16:07:46Z

index.html

-<li>Set the <a>resource timing buffer full flag</a> to true.</li>
+<ol style="list-style-type: lower-latin;">
+<li>Set <a>resource timing buffer full flag</a> to true.</li>
+<li>Queue a task to run <a>fire a buffer full event</a></li>


nit: period missing

npm1 · 2018-08-30T16:12:32Z

index.html

-</ol>
-</li>
+<li>If <a>resource timing buffer size limit</a> minus <a>resource timing buffer
+current size</a> is not larger than <a>resource timing secondary buffer current


larger than or equal to?

npm1 · 2018-08-30T16:15:37Z

index.html

-</li>
+<li>If <a>resource timing buffer size limit</a> minus <a>resource timing buffer
+current size</a> is not larger than <a>resource timing secondary buffer current
+size</a>, abort these steps.</li>


I think it might make sense to clear the secondary buffer even in the 'abort' case, so that we can keep listening to future entries even if the primary buffer was not cleared during this call. This is in line with the previous behavior, which would ignore entries when the buffer is full and the buffer full flag is true. Or even better but more complex, make the secondary buffer a queue, and if the queue becomes full and we want to add an entry then we pop the first in line (oldest entry). This way we maintain a list of 'recent' entries ready for when the primary buffer becomes available. WDYT? Also, I think we should add some entries from the secondary buffer even if not all of them can be added.

I think it makes sense to clear the secondary buffer either way.
I don't know if adding more complexity here (e.g. making it a queue) would provide a lot of benefit.

I guess we can add some entries from secondary to primary even if not all of them can fit, as long as we don't trigger RTbufferfull from within "fire a buffer full event"

npm1 · 2018-08-31T16:33:29Z

Thanks, LGTM! I should note a non-obvious change on this PR (not sure it's intentional?):

Before, setResourceTimingBufferSize would never fire resourcetimingbufferfull, and resourcetimingbufferfull would be fired immediately once the buffer becomes full when adding entries (if size is 3, fired after adding the 3rd entry). But this is buggy: if I setResourceTimingBufferSize to reduce the limit to something less than the current size then no event is fired, but then the next time we try to add an entry, there's no space so we fire the event but we are forced to drop the entry. I think it is for this reason that Chrome fires the event on setResourceTimingBufferSize when needed (avoids forcefully dropping the next entry).

With this PR, resourcetimingbufferfull is only fired (or more precisely, now a task is queued to fire it) when we try to add an entry and fail to do so (if size is 3, fired when failing to add the 4th entry). In particular it is still not fired on setResourceTimingBufferSize. But now doing this is fine because the secondary buffer should generally prevent us from dropping the entries. So this fixes the bug :)

yoavweiss · 2018-09-01T11:04:10Z

Thanks, LGTM! I should note a non-obvious change on this PR (not sure it's intentional?):

Before, setResourceTimingBufferSize would never fire resourcetimingbufferfull, and resourcetimingbufferfull would be fired immediately once the buffer becomes full when adding entries (if size is 3, fired after adding the 3rd entry). But this is buggy: if I setResourceTimingBufferSize to reduce the limit to something less than the current size then no event is fired, but then the next time we try to add an entry, there's no space so we fire the event but we are forced to drop the entry. I think it is for this reason that Chrome fires the event on setResourceTimingBufferSize when needed (avoids forcefully dropping the next entry).

With this PR, resourcetimingbufferfull is only fired (or more precisely, now a task is queued to fire it) when we try to add an entry and fail to do so (if size is 3, fired when failing to add the 4th entry). In particular it is still not fired on setResourceTimingBufferSize. But now doing this is fine because the secondary buffer should generally prevent us from dropping the entries. So this fixes the bug :)

This change is indeed intentional. Thanks for reviewing! :)

yoavweiss · 2018-09-01T11:04:45Z

Waiting on tests before merging

…sting entries

yoavweiss · 2018-09-02T05:20:59Z

@npm1 - looking at the code, I see that the event timing implementation follows the same model as resource timing, which makes me wonder if we should generalize the approach. Thoughts?

npm1 · 2018-09-04T17:01:01Z

I think the buffer mechanic is independent on the fact that this is resource timing so it might be worth generalizing. The only possible difference is whether we are forced to trigger the event async or not, but I think it would be better to be consistent and do async always even if not needed for a particular entryType. That said, I wonder if we really do need the clearing buffer mechanism for entry types that are only buffered until load. It seems to me that we want to stop buffering after load, so not sure how important this is in that case.

rniwa · 2018-09-04T21:33:59Z

index.html

+<li>Queue a task to run <a>fire a buffer full event</a>.</li>
+</ol>
+<li>If <a>resource timing secondary buffer current size</a> is less than
+<a>resource timing buffer size limit</a>, add <i>new entry</i> to the


We don't check condition in our implementation since the script wouldn't have a chance to clear the buffer until it receives the event. The secondary buffer is basically unbounded for us.

The point of the buffer size limit is to limit the amount of memory used. Having an unbounded secondary buffer makes that limit pointless.

Well, if we're delaying the event dispatching to the next task, how are author scripts supposed to know to clear the buffer? This unboundedness only occurs until the next task, and I don't see any reason why it would be bad. The script authors can just allocate a bunch of arrays to do the same if any.

While I agree that the unboundedness here would be short lived, I prefer to avoid scenarios where the event is called again on every microtask. So I prefer to define the buffer as bounded to the same size as the RT buffer to guaranty that if the developer either cleared the buffer or doubled its size, all entries will be successfully copied.

This can result in dropped entries when the buffer is too small, but I doubt that would happen in practice, and it makes the processing model simpler.

Well, if we're delaying the event dispatching to the next task, how are author scripts supposed to know to clear the buffer?

I'm not sure I understand the issue.
The proposed model is:

An entry which overflows the buffer, triggers the event queueing.

The entry as well as following ones go into the secondary buffer.

Once the event is fired, developers should make room in the primary buffer (either clear or bump its size)

Right after that, entries are copied from secondary to primary if possible

Do you see a problem with that model?

Oh now I remember why non-first resourcetimingbufferfull events are fired synchronously. This is so that we can ensure that either the buffer is emptied after a single task, or that the remaining entries are dropped on the floor.

Not that if we end up enqueuing more ResourceTiming entries as a result of dispatching resourcetimingbufferfull, we can fall into an infinite loop here but that's no worse than the current status quo, and this is no different from someone causing a mutual recursion between event listeners.

What is the benefit of adding all that complexity and edge cases? Why is it all of the sudden OK to fire the events synchronously the second time, but not the first time?

I think a simpler solution would be to basically tell people:

Don't set the buffer size to ridiculously small amounts. I doubt many people decrease the buffer sizes from their default size.

Clear or significantly increase (at least double) the buffer when it's full

I can see an argument in favor of adding console messages when this happens, so that developers can catch such scenarios, if they at all exist. But adding a ton of complexity in order to save a few entries in theory seem excessive.

Why is it all of the sudden OK to fire the events synchronously the second time, but not the first time?

Because we're already in a new task. There is no security or implementation issues from firing an event synchronously.

Because we're already in a new task

new task relative to what?

There is no security or implementation issues from firing an event synchronously.

Can you expand on why you objected to the original sync event firing that was in the spec? What were the issues there?

The problem is this. There is a lot of code in the browser engine where the resource load gets canceled (e.g. because the document is getting removed from a frame), and adding a resource timing entry in those timing is not permissible.

However, once this new timer fires, there is nothing like that happening down in the stack frame. So it's totally okay to synchronously fire events. Let me put this way. If it weren't okay to fire multiple resourcetimingbufferfull events, then it wouldn't have been okay to fire even one of them. It doesn't really matter how many events we fire. The important difference whether we fire any at all.

rniwa · 2018-09-04T21:36:18Z

index.html

+data-cite='!PERFORMANCE-TIMELINE-2/#dfn-performance-entry-buffer'>performance
+entry buffer</a>.</li>
+<li>Increment <a>resource timing buffer current size</a> by 1.</li>
+<li>Remove <i>entry</i> from <a>secondary entry buffer</a>.</li>


Looks like this is missing a step to fire resourcetimingbufferfull once the primary buffer becomes full again?

because the secondary buffer is bounded, and we assume developers do the right thing (either clear or double the buffer size), there's no need to fire the event again from here.

Note that one behavior change with this PR is that the event doesn't fire when the buffer is full, but fired when it overflows.

Okay, that needs to be changed once the secondary buffer's limit is lifted.

Even if we intend to keep an unbounded secondary buffer, for consistency we might want to fire the additional events async. I see no reason why this one would need be sync. So essentially we would just call the "add a PerformanceResourceTiming entry" algorithm.

Making those secondary events async would be fine with me.

rniwa · 2018-09-04T21:37:55Z

See
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Performance.cpp#L178
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Performance.cpp#L211

yoavweiss · 2018-09-05T07:18:17Z

See
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Performance.cpp#L178
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/Performance.cpp#L211

@rniwa I understand that the spec PR diverges from the current WebKit model. However, it seems like a reasonable compromise that enables capping the secondary buffer size without too much complexity.

yoavweiss · 2018-09-05T07:18:36Z

Tests for this PR as well as Chromium implementation are at https://chromium-review.googlesource.com/c/chromium/src/+/1200863

This change implements the processing model from PR 163[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. [1] w3c/resource-timing#163 Change-Id: I3fd901fa411dd128a723e77cd120f50974a775a8

rniwa · 2018-09-05T20:45:06Z

FYI, we're not going to change our behavior. Letting the secondary buffer overflow without ever letting the script react to it is simply not acceptable.

This change implements the processing model from PR 163[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. [1] w3c/resource-timing#163 Change-Id: I3fd901fa411dd128a723e77cd120f50974a775a8

yoavweiss · 2018-11-22T14:22:31Z

As discussed at TPAC, this approach is abandoned. Closing.

Fire bufferfull asynchronously

1b75de9

yoavweiss mentioned this pull request Aug 30, 2018

Clarify when entry is added to performance entry buffer #82

Closed

npm1 reviewed Aug 30, 2018

View reviewed changes

Tackle comments from npm

3a0fd18

Fixed a bug where the secondary buffer may be iterated beyond its exi…

059e4af

…sting entries

rniwa reviewed Sep 4, 2018

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Sep 5, 2018

Align resource timing buffer full processing to spec PR 163 web-platform-tests/wpt#12814

Closed

This was referenced Oct 9, 2018

Define buffering behaviors & hooks for queued (PerformanceObserver) entries w3c/performance-timeline#81

Closed

Fire resourcetimingbufferfull asynchronously - alternative approach #168

Merged

yoavweiss closed this Nov 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fire `resourcetimingbufferfull` asynchronously #163

Fire `resourcetimingbufferfull` asynchronously #163

yoavweiss commented Aug 30, 2018 •

edited by pr-preview bot

Loading

yoavweiss commented Aug 30, 2018

npm1 Aug 30, 2018

yoavweiss Aug 31, 2018

npm1 Aug 30, 2018

yoavweiss Aug 31, 2018

npm1 Aug 30, 2018

yoavweiss Aug 31, 2018

npm1 commented Aug 31, 2018

yoavweiss commented Sep 1, 2018

yoavweiss commented Sep 1, 2018

yoavweiss commented Sep 2, 2018

npm1 commented Sep 4, 2018

rniwa Sep 4, 2018

npm1 Sep 4, 2018

rniwa Sep 4, 2018

yoavweiss Sep 5, 2018

yoavweiss Sep 5, 2018

rniwa Sep 6, 2018

yoavweiss Sep 6, 2018

rniwa Sep 6, 2018

yoavweiss Sep 6, 2018 •

edited

Loading

rniwa Sep 6, 2018 •

edited

Loading

rniwa Sep 4, 2018

yoavweiss Sep 5, 2018

yoavweiss Sep 5, 2018

rniwa Sep 5, 2018

npm1 Sep 5, 2018

rniwa Sep 5, 2018

rniwa commented Sep 4, 2018

yoavweiss commented Sep 5, 2018

yoavweiss commented Sep 5, 2018

rniwa commented Sep 5, 2018

yoavweiss commented Nov 22, 2018

Fire resourcetimingbufferfull asynchronously #163

Fire resourcetimingbufferfull asynchronously #163

Conversation

yoavweiss commented Aug 30, 2018 • edited by pr-preview bot Loading

yoavweiss commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npm1 commented Aug 31, 2018

yoavweiss commented Sep 1, 2018

yoavweiss commented Sep 1, 2018

yoavweiss commented Sep 2, 2018

npm1 commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

rniwa Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa commented Sep 4, 2018

yoavweiss commented Sep 5, 2018

yoavweiss commented Sep 5, 2018

rniwa commented Sep 5, 2018

yoavweiss commented Nov 22, 2018

Fire `resourcetimingbufferfull` asynchronously #163

Fire `resourcetimingbufferfull` asynchronously #163

yoavweiss commented Aug 30, 2018 •

edited by pr-preview bot

Loading

yoavweiss Sep 6, 2018 •

edited

Loading

rniwa Sep 6, 2018 •

edited

Loading