Implement "onresourcetimingbufferfull" callback for Resource Timing.

RESOLVED FIXED in mozilla32

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: adrian.lungu89, Assigned: valentin)

Tracking

({dev-doc-needed})

unspecified
mozilla32
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 1002855
(Assignee)

Comment 2

5 years ago
Created attachment 8415075 [details] [diff] [review]
Implement onresourcetimingbufferfull callback for Resource Timing
(Assignee)

Updated

5 years ago
Assignee: nobody → valentin.gosu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

5 years ago
Attachment #8415075 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 8415166 [details] [diff] [review]
Implement onresourcetimingbufferfull callback for Resource Timing
(Assignee)

Updated

5 years ago
Attachment #8415075 - Attachment is obsolete: true
Attachment #8415075 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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?
(Assignee)

Comment 9

5 years ago
Created attachment 8415657 [details] [diff] [review]
Enforce buffer size for resource timing (part 1)
(Assignee)

Comment 10

5 years ago
Created attachment 8415658 [details] [diff] [review]
Implement onresourcetimingbufferfull callback (part 2)
(Assignee)

Updated

5 years ago
Attachment #8415166 - Attachment is obsolete: true
Attachment #8415166 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #8415657 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 13

5 years ago
(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?
(Assignee)

Comment 15

5 years ago
Created attachment 8419080 [details] [diff] [review]
Implement onresourcetimingbufferfull callback (part 2)
(Assignee)

Updated

5 years ago
Attachment #8415658 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 8419081 [details] [diff] [review]
Enforce buffer size for resource timing (part 1)
(Assignee)

Updated

5 years ago
Attachment #8415657 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
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
Last Resolved: 5 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)
(Assignee)

Comment 22

5 years ago
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?
(Assignee)

Comment 25

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.