Bug 84885 – [Resource Timing] Implement buffer limits

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[James Simonsen]

Resource timing is only supposed to buffer up to 150 entries by default. After that, it should emit an event that the buffer is full and drop entries until there is space.

[Ojan Vafai]

I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong.

I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). 

We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API.

[James Simonsen]

(In reply to comment #1)
> I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong.
> 
> I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). 
> 
> We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API.

That's fine with me. I wouldn't totally discount its use though. Every analytics tool will use this data. We put the buffer in, at their request, so that they could continue to load their script after load fires and still have access to the site's data. I'd imagine a significant percentage of sites use analytics.

[Annie Sullivan]

(In reply to comment #2)
> (In reply to comment #1)
> > I think we need to choose a smaller initial number. I know the spec says 150, but I think the spec is wrong. We can always change it later if it turns out we're wrong.
> > 
> > I'd be fine with the default buffer size being 0 or being something small enough to give time for the first script to run (e.g. 5-10). 
> > 
> > We shouldn't be wasting memory on every page for this when a incredibly tiny percentage of pages will actually use this API.
> 
> That's fine with me. I wouldn't totally discount its use though. Every analytics tool will use this data. We put the buffer in, at their request, so that they could continue to load their script after load fires and still have access to the site's data. I'd imagine a significant percentage of sites use analytics.

To add some stats, 46% of the Quantcast top million sites are currently using Analytics: http://trends.builtwith.com/analytics/Google-Analytics

[pdeng6]

Created attachment 161955 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161955 [details]
Patch

Attachment 161955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13732953

[Gyuyoung Kim]

Comment on attachment 161955 [details]
Patch

Attachment 161955 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13734631

[Peter Beverloo (cr-android ews)]

Comment on attachment 161955 [details]
Patch

Attachment 161955 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13746260

[Early Warning System Bot]

Comment on attachment 161955 [details]
Patch

Attachment 161955 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13746263

[Early Warning System Bot]

Comment on attachment 161955 [details]
Patch

Attachment 161955 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13739362

[James Simonsen]

View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review

> Source/WebCore/page/Performance.cpp:54
> +    m_resourceTimingBufferSize = defaultResourceTimingBufferSize;

This should be in the initializer list.

> Source/WebCore/page/Performance.cpp:154
> +        dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false));

This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full.

If the user clears the buffer during the event, then we should go ahead and record this resource.

I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full.

> Source/WebCore/page/Performance.h:99
> +    static const size_t defaultResourceTimingBufferSize = 150;

This doesn't need to be in the .h file.

> LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:8
> +            performance.webkitSetResourceTimingBufferSize(2);

2 -> buffer_size

> LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:24
> +                test_greater_than(count, 0, "onresourcetimingbufferfull should have been invoked!");

It should only happen once.

> LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:33
> +</html>

We need more test cases. For instance, if you shrink the buffer size, it shouldn't modify the buffer until clear is called. Also, something that reads and clears the buffer during the callback.

[pdeng6]

(In reply to comment #10)
> View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review
> 
> > Source/WebCore/page/Performance.cpp:54
> > +    m_resourceTimingBufferSize = defaultResourceTimingBufferSize;
> 
> This should be in the initializer list.
> 
> > Source/WebCore/page/Performance.cpp:154
> > +        dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false));
> 
> This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full.
> 
> If the user clears the buffer during the event, then we should go ahead and record this resource.
> 
Good point.
> I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full.
one exception is, what if a smaller buffer size is set when previous buffer size is not achieved yet? an extreme scenario is buffer size is set to 0 before any append() happen, I prefer fire the event in this condition if that is not fired on every resource that arrives after full.

in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group?

[pdeng6]

[1] http://www.w3.org/TR/2012/CR-resource-timing-20120522/#dom-performance-onresourcetimingbufferfull

[James Simonsen]

(In reply to comment #11)
> one exception is, what if a smaller buffer size is set when previous buffer size is not achieved yet?

Good example. Do you want to raise that on the list? I vote we fire the event once immediately in that case.

> an extreme scenario is buffer size is set to 0 before any append() happen, I prefer fire the event in this condition if that is not fired on every resource that arrives after full.

That's an interesting idea and is inline with what I initially proposed for Resource Timing (I hate this buffer). I think it should be brought up on the list though.

> in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group?

This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing.

[pdeng6]

 > > in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group?
> 
> This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing.

Agree this, one thing puzzles me is, item 5.1.20 of processing model in [1] describes, a temp buffer should be used to collect beyond limit timing entries, while nothing related mentioned in section 4.
I've raised this issue to web perf group, unfortunately no feedback.
my preference is drop the beyond timing entries as honor user's choice.

[James Simonsen]

(In reply to comment #14)
>  > > in addition, resource timing[1] said, "While executing the onresourcetimingbufferfull callback, PerformanceResourceTiming will continue to be collected beyond the maximum limit of the resources allowed in the PerformanceResourceTiming interface until...", seems this statement intends to ensure no resource timing entry loss when buffer is full. has it been discussed in web perf group?
> > 
> > This line doesn't apply to us. WebKit is single threaded. We can't add another resource to the buffer while the event is firing.

Actually, I thought of one: sync XHR. But that's a pretty extreme case and I really don't care whether or not it works.

[Tony Gentilcore]

Comment on attachment 161955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review

r- for the issues raised by simonjam. This is looking close, just needs a little more work.

> Source/WebCore/ChangeLog:3
> +        [Resource Timing]Implementation of reource timing buffer size restriction functionality

s/reourse/resource/

> LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:25
> +        <iframe id="frameContext" onload=onload_test() src="/w3c/webperf/resources/blank_page_green.htm" style="width: 250px; height: 250px;"></iframe>

These three tests don't look like they need to have an iframe. Could you please simplifying them and just put the onload handler in <body>.

[pdeng6]

Created attachment 168887 [details]
Patch

[pdeng6]

(In reply to comment #10)
> View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review
> 
> > Source/WebCore/page/Performance.cpp:54
> > +    m_resourceTimingBufferSize = defaultResourceTimingBufferSize;
> 
> This should be in the initializer list.
> 
Done.
> > Source/WebCore/page/Performance.cpp:154
> > +        dispatchEvent(Event::create(eventNames().webkitresourcetimingbufferfullEvent, false, false));
> 
> This event is only supposed to fire once when it becomes full, not on every resource that arrives after it's full.
> 
> If the user clears the buffer during the event, then we should go ahead and record this resource.
> 
> I started a thread about this on web-perf. My goal for the thread was to fire the event after append() makes it full.
> 
now, it event fire after append() makes it full, and shrink a less size.
> > Source/WebCore/page/Performance.h:99
> > +    static const size_t defaultResourceTimingBufferSize = 150;
> 
> This doesn't need to be in the .h file.
> 
Done.
> > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:8
> > +            performance.webkitSetResourceTimingBufferSize(2);
> 
> 2 -> buffer_size
> 
Done
> > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:24
> > +                test_greater_than(count, 0, "onresourcetimingbufferfull should have been invoked!");
> 
> It should only happen once.
> 
Done, happen once now.
> > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_on_resource_timing_buffer_full.html:33
> > +</html>
> 
> We need more test cases. For instance, if you shrink the buffer size, it shouldn't modify the buffer until clear is called. Also, something that reads and clears the buffer during the callback.

Good idea, I add some typical use case of this

[pdeng6]

(In reply to comment #16)
> (From update of attachment 161955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161955&action=review
> 
> r- for the issues raised by simonjam. This is looking close, just needs a little more work.
> 
> > Source/WebCore/ChangeLog:3
> > +        [Resource Timing]Implementation of reource timing buffer size restriction functionality
> 
> s/reourse/resource/
> 
> > LayoutTests/http/tests/w3c/webperf/proposal/resource-timing/test_resource_timing_buffer_size_restriction.html:25
> > +        <iframe id="frameContext" onload=onload_test() src="/w3c/webperf/resources/blank_page_green.htm" style="width: 250px; height: 250px;"></iframe>
> 
> These three tests don't look like they need to have an iframe. Could you please simplifying them and just put the onload handler in <body>.
Done, thanks!

[WebKit Review Bot]

Comment on attachment 168887 [details]
Patch

Attachment 168887 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14389118

New failing tests:
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_on_shrink_buffer_size.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_size_restriction.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_clear_resource_timing_functionality.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_full_when_populate_entries.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_store_and_clear_during_callback.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_timing_buffer_full_when_shrink_buffer_size.html

[Tony Gentilcore]

Comment on attachment 168887 [details]
Patch

You just need to skip the new tests until it is enabled. Everything else looks good.

[pdeng6]

Created attachment 169376 [details]
Patch

[pdeng6]

(In reply to comment #21)
> (From update of attachment 168887 [details])
> You just need to skip the new tests until it is enabled. Everything else looks good.
skip all the new test cases.
I change test cases to match upstream, including prefix handle, and folder.
thanks.

[Tony Gentilcore]

Comment on attachment 169376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169376&action=review

> Source/WebCore/page/Performance.cpp:167
> +    if (isResourceTimingBufferFull())

For efficiency, can't this go above the creation of the entry?

[pdeng6]

Created attachment 169510 [details]
Patch

[pdeng6]

(In reply to comment #24)
> (From update of attachment 169376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169376&action=review
> 
> > Source/WebCore/page/Performance.cpp:167
> > +    if (isResourceTimingBufferFull())
> 
> For efficiency, can't this go above the creation of the entry?
Good idea, done :)

[WebKit Review Bot]

Comment on attachment 169510 [details]
Patch

Clearing flags on attachment: 169510

Committed r131829: <http://trac.webkit.org/changeset/131829>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.