Closed Bug 936813 Opened 7 years ago Closed 7 years ago

Implement "onresourcetimingbufferfull" callback for Resource Timing.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: adrian.lungu89, Assigned: valentin)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

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
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
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 1002855
Assignee: nobody → valentin.gosu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8415075 - Flags: review?(bzbarsky)
Attachment #8415075 - Attachment is obsolete: true
Attachment #8415075 - Flags: review?(bzbarsky)
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
Attachment #8415166 - Flags: review?(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?
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?
Flags: needinfo?(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....
Flags: needinfo?(bzbarsky)
So maybe what you should do is a patch under that one that adds the actual capping of the buffer size?
Attachment #8415166 - Attachment is obsolete: true
Attachment #8415166 - Flags: review?(bzbarsky)
Attachment #8415657 - Flags: review?(bzbarsky)
Attachment #8415658 - Flags: review?(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.
Attachment #8415657 - Flags: review?(bzbarsky) → review+
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!
Attachment #8415658 - Flags: review?(bzbarsky) → review+
(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);
};
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?
Attachment #8415658 - Attachment is obsolete: true
Attachment #8415657 - Attachment is obsolete: true
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 <valentin.gosu@gmail.com> - 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 <valentin.gosu@gmail.com> - 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.
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.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/d7e4adb277f9
https://hg.mozilla.org/mozilla-central/rev/9fda399b4e88
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
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 )
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.
Flags: needinfo?(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?
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.