Fire `resourcetimingbufferfull` asynchronously - alternative approach #168

yoavweiss · 2018-10-10T09:07:51Z

This is an alternative approach to #163 in order to resolve #141 and #96.

Handwavy processing

The secondary buffer is unlimited.
The event is fired async once, and if the buffer size exceeds its limits after the event fired, it's continuously fired synchronously until the size does not exceed.
If the event handler does not reduce the buffer size or increases the limit, all entries that exceed the buffer max size get dropped.

/cc @rniwa @tdresser @npm1

Preview | Diff

igrigorik · 2018-10-18T06:59:30Z

@yoavweiss there is a merge conflict in the PR, could you take a quick look?

@rniwa @tdresser @npm1 friendly bump :)

rniwa · 2018-10-18T21:59:54Z

index.html

+<code>bubbles</code> attribute initialized to true.</li>
+<li>Let <i>excess entries after</i> be the <a>resource timing buffer current size</a> 
+minus <a>resource timing buffer size limit</a>.</li>
+<li>If <i>excess entries before</i> equals <i>excess entries after</i>, then while <a>resource timing buffer current size</a> is larger than <a>resource timing buffer size limit</a>, run the following substeps:


This isn't quite right. There is no guarantee that the secondary buffer is empty at this point.
We need to start dropping things from the secondary buffer first.

In step 2 of fire the buffer full event we clear out the secondary buffer. Then if the number of entries exceeds the buffer size, we fire the event synchronously, and if the buffer size remained the same, we drop entries.

How could the secondary buffer be non-empty at this point? Are you suggesting that "add a PerformanceResourceTiming entry" call happened while these sync steps are running?

As I mentioned during F2F, firing of resourcetimingbufferfull event itself can result in more resource timing entries to be added; e.g. via a sync XHR.

@rniwa - Fixed the processing to fire the copying from the secondary buffer after the sync event firing (and deleting everything that exceeds if the buffer is equal or larger).
PTAL

npm1 · 2018-11-12T22:44:40Z

We decided to go with this alternative, right? Is this ready for review now?

yoavweiss · 2018-11-20T10:30:42Z

We decided to go with this alternative, right? Is this ready for review now?

Not yet, as @rniwa's comments are still pending. Looking at that now

yoavweiss · 2018-11-20T10:31:11Z

@yoavweiss there is a merge conflict in the PR, could you take a quick look?

Missed that comment... Rebasing now

yoavweiss · 2018-11-20T10:52:38Z

We decided to go with this alternative, right? Is this ready for review now?

Ready for review now :)

author Yoav Weiss <yoav@yoav.ws> 1535634012 +0200 committer Yoav Weiss <yoav@yoav.ws> 1542727833 +0100 Fire bufferfull asynchronously Tackle comments from npm Fixed a bug where the secondary buffer may be iterated beyond its existing entries Alternative processing model Addressed review comments rebase leftovers more rebase leftovers remove secondary buffer size accounting improved note improved note 2 Remove buffer accounting 2

yoavweiss · 2018-11-20T15:42:22Z

Had to manually rebase and force push due to merge shenanigans. Should be stable now...

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

index.html

npm1

I think we need to modify the copy secondary buffer algorithm a bit.

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

index.html

npm1

I think we need to be careful not to directly add entries to the primary buffer if the secondary buffer has some.

index.html

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

index.html

This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. This is a reland of [2] which is a reland of [3] (but with nicer tests). [1] w3c/resource-timing#168 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1350950 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1345269 Bug: 908181, 908414 Change-Id: I3a6c6e9d6a9aa5b5f907d1e86bec701ff2fa022d Reviewed-on: https://chromium-review.googlesource.com/c/1373819 Reviewed-by: Nicolás Peña Moreno <npm@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#615929}

…sing to spec PR 168 (take 3), a=testonly Automatic update from web-platform-tests Align resource timing buffer full processing to spec PR 168 (take 3) This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. This is a reland of [2] which is a reland of [3] (but with nicer tests). [1] w3c/resource-timing#168 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1350950 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1345269 Bug: 908181, 908414 Change-Id: I3a6c6e9d6a9aa5b5f907d1e86bec701ff2fa022d Reviewed-on: https://chromium-review.googlesource.com/c/1373819 Reviewed-by: Nicolás Peña Moreno <npm@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#615929} -- wpt-commits: 79001b1d0d4a111b7f44608404d7b5504d755a1c wpt-pr: 14479

…sing to spec PR 168, a=testonly Automatic update from web-platform-tests Align resource timing buffer full processing to spec PR 168 This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. [1] w3c/resource-timing#168 Change-Id: I3a57196f10e0b4cf2bae5662b0e075673a0c2d80 Reviewed-on: https://chromium-review.googlesource.com/c/1345269 Commit-Queue: Yoav Weiss <yoavweisschromium.org> Commit-Queue: Yoav Weiss <yoavyoav.ws> Reviewed-by: Nicolás Peña Moreno <npmchromium.org> Cr-Commit-Position: refs/heads/master{#610667} -- wpt-commits: 06720fac129ee8916f1a9309ebc0bfa134604bfe wpt-pr: 14167 UltraBlame original commit: 978c4475bce230e30586091504c147e0c7aeb127

…l processing to spec PR 168", a=testonly Automatic update from web-platform-tests Revert "Align resource timing buffer full processing to spec PR 168" This reverts commit 579d65014c9d6cc248ec1de47013bf6c20c72158. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 610667 as the culprit for flakes in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNTc5ZDY1MDE0YzlkNmNjMjQ4ZWMxZGU0NzAxM2JmNmMyMGM3MjE1OAw Sample Failed Build: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak/26746 Sample Failed Step: webkit_layout_tests Sample Flaky Test: external/wpt/resource-timing/buffer-full-store-and-clear-during-callback.html Original change's description: > Align resource timing buffer full processing to spec PR 168 > > This change implements the processing model from PR 168[1], when > it comes to setResourceTimingBufferSize(), clearResourceTimings() > and the firing of the resourcetimingbufferfull event. > > [1] w3c/resource-timing#168 > > Change-Id: I3a57196f10e0b4cf2bae5662b0e075673a0c2d80 > Reviewed-on: https://chromium-review.googlesource.com/c/1345269 > Commit-Queue: Yoav Weiss <yoavweisschromium.org> > Commit-Queue: Yoav Weiss <yoavyoav.ws> > Reviewed-by: Nicolás Peña Moreno <npmchromium.org> > Cr-Commit-Position: refs/heads/master{#610667} Change-Id: I0840bd9b763030b6e200e8f9a94c73c7982044a0 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 908181 Reviewed-on: https://chromium-review.googlesource.com/c/1349498 Cr-Commit-Position: refs/heads/master{#610683} -- wpt-commits: 40e50ecff750feff8e62dc98a72e9bca3a8c94cf wpt-pr: 14219 UltraBlame original commit: 2d0334e10ba175a600b89f263f3377fe6b821b8d

…sing to spec PR 168 (reland), a=testonly Automatic update from web-platform-tests Align resource timing buffer full processing to spec PR 168 (reland) This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1345269 (but with nicer tests). [1] w3c/resource-timing#168 Change-Id: I10431bfda7f79b484c7ee5c608bb8a360d905339 Bug: 908181, 908414 Reviewed-on: https://chromium-review.googlesource.com/c/1350950 Reviewed-by: Nicolás Peña Moreno <npmchromium.org> Commit-Queue: Yoav Weiss <yoavweisschromium.org> Cr-Commit-Position: refs/heads/master{#611174} -- wpt-commits: cccb52680a601f4de432124092c808e91fad8a15 wpt-pr: 14234 UltraBlame original commit: af450646a96991b8bf823da24451f8da40bd7063

…l processing to spec PR 168 (reland)", a=testonly Automatic update from web-platform-tests Revert "Align resource timing buffer full processing to spec PR 168 (reland)" This reverts commit 6dd060768d1c1183e0d29e422a2213f106d98299. Reason for revert: suspect causing WebRtcDisableEncryptionFlagBrowserTest.VerifyEncryption to be flaky (FindIt's report) BUG:chromium:909704 Original change's description: > Align resource timing buffer full processing to spec PR 168 (reland) > > This change implements the processing model from PR 168[1], when > it comes to setResourceTimingBufferSize(), clearResourceTimings() > and the firing of the resourcetimingbufferfull event. > > This is a reland of > https://chromium-review.googlesource.com/c/chromium/src/+/1345269 > (but with nicer tests). > > [1] w3c/resource-timing#168 > > Change-Id: I10431bfda7f79b484c7ee5c608bb8a360d905339 > Bug: 908181, 908414 > Reviewed-on: https://chromium-review.googlesource.com/c/1350950 > Reviewed-by: Nicolás Peña Moreno <npmchromium.org> > Commit-Queue: Yoav Weiss <yoavweisschromium.org> > Cr-Commit-Position: refs/heads/master{#611174} TBR=npmchromium.org,yoavweisschromium.org Change-Id: Ia4bf26c6be1e7c4ae35b904bac25fe031f854604 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 908181, 908414 Reviewed-on: https://chromium-review.googlesource.com/c/1354123 Reviewed-by: Ned Nguyen <nednguyengoogle.com> Commit-Queue: Ned Nguyen <nednguyengoogle.com> Cr-Commit-Position: refs/heads/master{#611722} -- wpt-commits: ec13cf1ca3abf13ae1004003e791fd9937be0b49 wpt-pr: 14282 UltraBlame original commit: 4b81c4c2722a0aa064267fcd02c9f4a0a9d0fb0d

…sing to spec PR 168, a=testonly Automatic update from web-platform-tests Align resource timing buffer full processing to spec PR 168 This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. [1] w3c/resource-timing#168 Change-Id: I3a57196f10e0b4cf2bae5662b0e075673a0c2d80 Reviewed-on: https://chromium-review.googlesource.com/c/1345269 Commit-Queue: Yoav Weiss <yoavweisschromium.org> Commit-Queue: Yoav Weiss <yoavyoav.ws> Reviewed-by: Nicolás Peña Moreno <npmchromium.org> Cr-Commit-Position: refs/heads/master{#610667} -- wpt-commits: 06720fac129ee8916f1a9309ebc0bfa134604bfe wpt-pr: 14167 UltraBlame original commit: 01034b130bc60235f297cd7799273bad7a5d70a5

…l processing to spec PR 168", a=testonly Automatic update from web-platform-tests Revert "Align resource timing buffer full processing to spec PR 168" This reverts commit 579d65014c9d6cc248ec1de47013bf6c20c72158. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 610667 as the culprit for flakes in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNTc5ZDY1MDE0YzlkNmNjMjQ4ZWMxZGU0NzAxM2JmNmMyMGM3MjE1OAw Sample Failed Build: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak/26746 Sample Failed Step: webkit_layout_tests Sample Flaky Test: external/wpt/resource-timing/buffer-full-store-and-clear-during-callback.html Original change's description: > Align resource timing buffer full processing to spec PR 168 > > This change implements the processing model from PR 168[1], when > it comes to setResourceTimingBufferSize(), clearResourceTimings() > and the firing of the resourcetimingbufferfull event. > > [1] w3c/resource-timing#168 > > Change-Id: I3a57196f10e0b4cf2bae5662b0e075673a0c2d80 > Reviewed-on: https://chromium-review.googlesource.com/c/1345269 > Commit-Queue: Yoav Weiss <yoavweisschromium.org> > Commit-Queue: Yoav Weiss <yoavyoav.ws> > Reviewed-by: Nicolás Peña Moreno <npmchromium.org> > Cr-Commit-Position: refs/heads/master{#610667} Change-Id: I0840bd9b763030b6e200e8f9a94c73c7982044a0 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 908181 Reviewed-on: https://chromium-review.googlesource.com/c/1349498 Cr-Commit-Position: refs/heads/master{#610683} -- wpt-commits: 40e50ecff750feff8e62dc98a72e9bca3a8c94cf wpt-pr: 14219 UltraBlame original commit: 5b2b6bddba0b1bd55a1e59baf3be2ffb01d770c2

…sing to spec PR 168 (reland), a=testonly Automatic update from web-platform-tests Align resource timing buffer full processing to spec PR 168 (reland) This change implements the processing model from PR 168[1], when it comes to setResourceTimingBufferSize(), clearResourceTimings() and the firing of the resourcetimingbufferfull event. This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1345269 (but with nicer tests). [1] w3c/resource-timing#168 Change-Id: I10431bfda7f79b484c7ee5c608bb8a360d905339 Bug: 908181, 908414 Reviewed-on: https://chromium-review.googlesource.com/c/1350950 Reviewed-by: Nicolás Peña Moreno <npmchromium.org> Commit-Queue: Yoav Weiss <yoavweisschromium.org> Cr-Commit-Position: refs/heads/master{#611174} -- wpt-commits: cccb52680a601f4de432124092c808e91fad8a15 wpt-pr: 14234 UltraBlame original commit: 359a084be7533d19531898c387f5989029c73f84

…l processing to spec PR 168 (reland)", a=testonly Automatic update from web-platform-tests Revert "Align resource timing buffer full processing to spec PR 168 (reland)" This reverts commit 6dd060768d1c1183e0d29e422a2213f106d98299. Reason for revert: suspect causing WebRtcDisableEncryptionFlagBrowserTest.VerifyEncryption to be flaky (FindIt's report) BUG:chromium:909704 Original change's description: > Align resource timing buffer full processing to spec PR 168 (reland) > > This change implements the processing model from PR 168[1], when > it comes to setResourceTimingBufferSize(), clearResourceTimings() > and the firing of the resourcetimingbufferfull event. > > This is a reland of > https://chromium-review.googlesource.com/c/chromium/src/+/1345269 > (but with nicer tests). > > [1] w3c/resource-timing#168 > > Change-Id: I10431bfda7f79b484c7ee5c608bb8a360d905339 > Bug: 908181, 908414 > Reviewed-on: https://chromium-review.googlesource.com/c/1350950 > Reviewed-by: Nicolás Peña Moreno <npmchromium.org> > Commit-Queue: Yoav Weiss <yoavweisschromium.org> > Cr-Commit-Position: refs/heads/master{#611174} TBR=npmchromium.org,yoavweisschromium.org Change-Id: Ia4bf26c6be1e7c4ae35b904bac25fe031f854604 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 908181, 908414 Reviewed-on: https://chromium-review.googlesource.com/c/1354123 Reviewed-by: Ned Nguyen <nednguyengoogle.com> Commit-Queue: Ned Nguyen <nednguyengoogle.com> Cr-Commit-Position: refs/heads/master{#611722} -- wpt-commits: ec13cf1ca3abf13ae1004003e791fd9937be0b49 wpt-pr: 14282 UltraBlame original commit: 7cf604bd53deaa8a0dc5b4f12f9f4dd99f955b0c