936813 - Implement "onresourcetimingbufferfull" callback for Resource Timing.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Adrian Lungu]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Comment 1

[Adrian Lungu]

Before Resource timing will be prefed on, the "onresourcetimingbufferfull" callback must be implemented.
Its behavior is not clear enough in the spec. There was a thread started on the w3c mailing list about it:
http://lists.w3.org/Archives/Public/public-web-perf/2013Sep/0026.html

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Implement onresourcetimingbufferfull callback for Resource Timing

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Implement onresourcetimingbufferfull callback for Resource Timing

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8415166 [details] [diff] [review]
Implement onresourcetimingbufferfull callback for Resource Timing

Fixed failure in test_toJSON.html because the eventhandler was undefined.
https://tbpl.mozilla.org/?tree=Try&rev=39e343c1ce9a

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8415166 [details] [diff] [review]
Implement onresourcetimingbufferfull callback for Resource Timing

So I thought the point of this stuff was to avoid having an unbounded-size buffer, no?  I don't see other ever limiting how much we store; just how much we're willing to report as storing.  Am I just missing something?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Indeed, it seems that the buffer size is only used for the number of entries we report on. I hadn't realized that was the case. Should I fix it in this patch, or should I file another bug?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think it doesn't make sense to do this separately; it's hard to evaluate the correctness of firing this event without the other part....

Comment 8

[Boris Zbarsky [:bzbarsky]]

So maybe what you should do is a patch under that one that adds the actual capping of the buffer size?

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Enforce buffer size for resource timing (part 1)

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: Implement onresourcetimingbufferfull callback (part 2)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8415657 [details] [diff] [review]
Enforce buffer size for resource timing (part 1)

> nsPerformance::GetEntries(nsTArray<nsRefPtr<PerformanceEntry> >& retval)

Why can't this just become:

  retval = mEntries;

?  

> nsPerformance::SetResourceTimingBufferSize(uint64_t maxSize)

So I think what we implement here is the letter of the spec, but not the intent, in the sense that we require an explicit clear call before a _larger_ buffer size takes effect, no?  That seems really weird; do other UAs do that?  If they do not, please raise this as a spec issue; the spec is way too unclear on the actual processing model for this method.

>+  // Don't add the entry if the buffer if full

s/if full/is full/

r=me with those fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8415658 [details] [diff] [review]
Implement onresourcetimingbufferfull callback (part 2)

>+  // The event handler needs to be initialized for the attributes to match

Which attributes to match what?  Might want a less cryptic comment here.

r=me with that.  Thank you!

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> So I think what we implement here is the letter of the spec, but not the
> intent, in the sense that we require an explicit clear call before a
> _larger_ buffer size takes effect, no?  That seems really weird; do other
> UAs do that?  If they do not, please raise this as a spec issue; the spec is
> way too unclear on the actual processing model for this method.

I have checked, and indeed, Chrome seems to increase the buffer immediately. After reviewing the spec more carefully it seems that Step 20 of http://www.w3.org/TR/resource-timing/#processing-model describes this behaviour. I have sent an email raising the spec issue, and also if calling setResourceTimingBufferSize should trigger a bufferfull event leading to a loop: (currently the case in Chrome)

performance.setResourceTimingBufferSize(1);
performance.onresourcebufferfull = function() {
    performance.setResourceTimingBufferSize(1);
};

Comment 14

[Boris Zbarsky [:bzbarsky]]

Thanks for raising the spec issues!  I followed up on one of them, because as far as I can tell step 20 is so underdefined it's not clear what it really means as the spec is currently written.

In the meantime, let's assume that what the spec means to say is the following:

1)  There is an internal "buffer size" slot.
2)  Calling setResourceTimingBufferSize sets the value of that slot but does nothing else.
3)  In step 20 of the processing model, if the number of entries in the buffer is at least
    "buffer size", then fire the event.

Make sense?

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Attachment: Implement onresourcetimingbufferfull callback (part 2)

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Attachment: Enforce buffer size for resource timing (part 1)

Comment 17

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e4adb277f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fda399b4e88

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Regression: Mozilla-Inbound-Non-PGO - Tp5 No Network Row Major MozAfterPaint (Non-Main Normal Network IO Bytes) - WINNT 6.1 (ix) - 63% increase
-----------------------------------------------------------------------------------------------------------------------------------------------
    Previous: avg 108451158.333 stddev 20835323.691 of 12 runs up to revision 62d2d22055e8
    New     : avg 176762166.667 stddev 11534886.915 of 12 runs since revision 9fda399b4e88
    Change  : +68311008.333 (63% / z=3.279)
    Graph   : http://mzl.la/1iDrCAY

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=62d2d22055e8&tochange=9fda399b4e88

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/d7e4adb277f9
    : Valentin Gosu <[e-mail]> - Bug 936813 - Enforce buffer size for resource timing (part 1) r=bz
    : http://bugzilla.mozilla.org/show_bug.cgi?id=936813

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/9fda399b4e88
    : Valentin Gosu <[e-mail]> - Bug 936813 - Implement onresourcetimingbufferfull callback (part 2) r=bz
    : http://bugzilla.mozilla.org/show_bug.cgi?id=936813

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=936813 - Implement "onresourcetimingbufferfull" callback for Resource Timing.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Boris, do you think the regression in comment 18 might have been caused by extending DOMEventTargetHelper? I'm not sure what else could have caused it.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7e4adb277f9
https://hg.mozilla.org/mozilla-central/rev/9fda399b4e88

Comment 21

[Boris Zbarsky [:bzbarsky]]

Wait.  Did the modified patches ever get reviewed?  The review in comment 11 was conditional on a followup being filed to address the spec issue; once we decided on nontrivial code changes instead, those code changes kinda need review, no?

Comment 22

[Valentin Gosu [:valentin] (he/him)]

I addressed your comments, and the spec changes were in line with in line with the code we had. The only other change was removing mBufferSizeSet, in the first patch, since it wasn't necessary anymore ( comment 14 )

Comment 23

[Boris Zbarsky [:bzbarsky]]

So looking at the spec post-changes, the spec only fires the event if mEntries.Length() == mPrimaryBufferSize at the end of AddEntry.

I guess your >= check that landed is equivalent, since mEntries.Length() > mPrimaryBufferSize would mean that on entry to the method mEntries.Length() >= mPrimaryBufferSize and we'd have taken the early return.  But it's worth either commenting why our code is not obviously matching the spec or changing the code to match the spec (possibly with an assert that mEntries.Length() <= mPrimaryBufferSize after the InsertElementSorted() call.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, and to be clear, I re-reviewed the patches; comment 23 is the only issue I see.  As for the performance regression, I don't see anything in these patches that should obviously have caused that.  Is that confirmed?

Comment 25

[Valentin Gosu [:valentin] (he/him)]

Thanks for that. I also talked to smaug about whether extending DOMEventTargetHelper could have a performance impact, and he also said no.
The graph http://mzl.la/1iDrCAY seems to indicate that the issue is gone now, and it was a coincidence that it happened when it did.