Turn on Resource Timing

RESOLVED FIXED in mozilla35

Status

()

Core
Networking
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Depends on: 2 bugs, {dev-doc-needed, meta, relnote})

unspecified
mozilla35
dev-doc-needed, meta, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 35+)

Details

Attachments

(2 attachments)

Comment hidden (empty)
Depends on: 822480
So we got most of the Resource Timing API landed (pref'd off) in bug 822480.  Bug 822480 comment 1 and bug 936814 comment 1 make it sound like we need the "onresourcetimingbufferfull" callback and/or cross-origin support before we pref it on, but maybe we're ok with less than 100% feature set to start.  JST, Jonas, do you have an opinion on what we need to pref it on?
Flags: needinfo?(jst)
Jason, please send an "Intent to ship" message to dev.platform and we'll iron out the details there. In general I'm all for shipping complete implementations, but at the same time shipping a compliant though not complete implementation seems better than not shipping a feature at all.
Flags: needinfo?(jst)
The most important piece is that people can feature detect what's implemented and what isn't. If the resource timing data simply isn't showing up for some requests then I think that's fine. If it's showing up but is all zeros then that's slightly worse.
> If the resource timing data simply isn't showing up for some requests then I think that's fine. If it's showing up but is all zeros then that's slightly worse.

Valentin: which is these is happening now for cross-origin requests?

We haven't heard much from the dev.platform thread, so maybe we should just knock off the remaining 2 bugs and then turn on.
Flags: needinfo?(valentin.gosu)
The IsSameOriginAsReferral/CheckRedirectCrossOrigin methods aren't fully implemented, so it will not take the Timing-Allow-Origin header into account.
As a consequence, the resources will appear in the getEntries, but several attributes will be set to 0.

http://dxr.mozilla.org/mozilla-central/source/dom/base/PerformanceResourceTiming.h#63

If we close the 2 remaining bugs, everything should be properly implemented.
Flags: needinfo?(valentin.gosu)
Depends on: 1006575
Depends on: 1009360
Depends on: 1021221
Keywords: dev-doc-needed

Comment 6

3 years ago
I was wondering if there's any way to know when window.performance.getEntries is likely to be available in FF.  I'm working on a tool which uses the resource timing api, and we're approaching the point where we'll be creating marketing materials, etc.  Currently we're only able to say the tools works in current versions of Chrome and IE, but we'd like to claim FF support as well.  Is following this issue the best way to stay current on when RTS will be released in FF?
(In reply to sgrock from comment #6)
Resource timing has been implemented, and can be enabled in about:config by setting dom.enable_resource_timing to true. It will probably be enabled by default after closing Bug 936814.
You can watch this bug to see when the feature is turned on, and the bugs blocking it to see progress on individual issues.
Depends on: 1047848
Depends on: 1064706
Created attachment 8491895 [details] [diff] [review]
Turn on Resource Timing

Boris, since bug 936814 is resolved, do you think we should flip the pref, and turn it on by default?
Attachment #8491895 - Flags: review?(bzbarsky)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Where are we on passing the test suite, assuming there is a test suite?

As in, how non-buggy do we think this is in practice?
Flags: needinfo?(valentin.gosu)
http://w3c-test.org/resource-timing/test_resource_timing.html
16 PASS and 5 FAIL
We seem to be failing the same tests that Chrome is.

I don't expect this to be too buggy. All of the bugs I have encountered are blocking this bug.
Flags: needinfo?(valentin.gosu)
> 16 PASS and 5 FAIL

OK.  Have you looked into the failures?  Are they bugs in our code or in the test?

> I don't expect this to be too buggy. 

Is it not-buggy enough that we're willing to freeze the existing behaviors once websites start depending on them?

What does our own test coverage for this look like?
Also, please file a spec bug about the test suite using the non-standard innerText property, though that doesn't affect the test results.
Oh, and the test should probably test object/embed to.  File spec bugs on that as well?
Also, you probably want to send an intent to ship mail.
(In reply to Boris Zbarsky [:bz] from comment #11)
> > 16 PASS and 5 FAIL
> 
> OK.  Have you looked into the failures?  Are they bugs in our code or in the
> test?

One of them could be due to comparing floats with the same value.
Not sure why the resource_timing_test0.xml test fails. The entry does appear in getEntries() and the fields seem to be ok.
The html entry and png entry also seem fine, but the tests for them fail.

It's possible these are bugs in the test, but it needs more investigation.

> > I don't expect this to be too buggy. 
> 
> Is it not-buggy enough that we're willing to freeze the existing behaviors
> once websites start depending on them?
> 
> What does our own test coverage for this look like?

We have 2 mochitests for this covering resource timing:

dom/tests/mochitest/general/test_resource_timing.html
dom/tests/mochitest/general/test_resource_timing_cross_origin.html
Depends on: 1070146
Thanks!

Sounds like we should figure out what's up with the official test suite failures, add a test for <script> to our tests, and send that intent to ship mail.  Looks pretty good with that done.
Comment on attachment 8491895 [details] [diff] [review]
Turn on Resource Timing

r=me once the prereqs are done, unless the intent to ship thread disagrees somehow.
Attachment #8491895 - Flags: review?(bzbarsky) → review+
I have filed the following bugs for the w3c test:

https://github.com/w3c/web-platform-tests/issues/1256 - for the use of the non-standard innerText
https://github.com/w3c/web-platform-tests/issues/1257 - test assumes secureConnectionStart is undefined
https://github.com/w3c/web-platform-tests/issues/1258 - test calls getEntriesByName with wrong name for xml file

These cover all the failing tests.
Excellent, thank you!
https://tbpl.mozilla.org/?tree=Try&rev=71a9cb02ab26

I think we can land this when inbound is open again.
No objections on the intent to ship mail.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42938fd0288
Keywords: checkin-needed
sorry had to backout this change for web platform 4 test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2644218&repo=mozilla-inbound
I need to update the tests so they don't expect failure. Thanks.
Created attachment 8497550 [details] [diff] [review]
Fix web-platform-tests for resource-timing and remove expect FAIL
Attachment #8497550 - Flags: review?(bzbarsky)
Comment on attachment 8497550 [details] [diff] [review]
Fix web-platform-tests for resource-timing and remove expect FAIL

>+  [window.performance.getEntriesByName("http://.../resource_timing_test0.xml") returns a PerformanceEntry object]

This is <https://github.com/w3c/web-platform-tests/issues/1258>, right?

>+                     actualEntry.secureConnectionStart == 0) &&

And this is <https://github.com/w3c/web-platform-tests/issues/1257>?

Can this be done via an annotation in the .ini instead of modifying the test?  I _think_ the tests themselves are not supposed to be modified in our tree except via mass-import from upstream....

r=me modulo that.
Flags: needinfo?(james)
(In reply to Boris Zbarsky [:bz] from comment #25)
> Comment on attachment 8497550 [details] [diff] [review]
> Fix web-platform-tests for resource-timing and remove expect FAIL
> 
> >+  [window.performance.getEntriesByName("http://.../resource_timing_test0.xml") returns a PerformanceEntry object]
> 
> This is <https://github.com/w3c/web-platform-tests/issues/1258>, right?
> 
> >+                     actualEntry.secureConnectionStart == 0) &&
> 
> And this is <https://github.com/w3c/web-platform-tests/issues/1257>?
> 
> Can this be done via an annotation in the .ini instead of modifying the
> test?  I _think_ the tests themselves are not supposed to be modified in our
> tree except via mass-import from upstream....
> 
> r=me modulo that.

If that's the case I'll annotate the .ini files, and submit the pull requests to upstream.
Thanks!
Attachment #8497550 - Flags: review?(bzbarsky) → review+
Yep, you can't change the upstream tests in our tree and as any changes you do make will be overwritten at the next sync.

I have a system in staging that allows us to run our own tests outside of the sync; the idea is that you can then fix the test in the Mozilla tree and as part of the next sync we will automatically create and merge a PR for your change. The PR-creating part of that doesn't actually exist yet, so please don't wait on this to upstream your changes here, but I am working on making things better :)

Thanks for fixing the tests. If you submit this patch upstream we can carry forward the r+ from bz and you don't need to wait to get a second round of review for the PR. Feel free to ping me (or needinfo me here) and I'll make sure your change gets merged.
Flags: needinfo?(james)
Thanks James! I think it's preferable to fix the test, as those failure messages contain timestamps, which makes it difficult to add exceptions using the .ini files.

I have submitted a pull request: https://github.com/w3c/web-platform-tests/pull/1266
OK, merged the PR. I'll update our local copy soon.

If the test titles end up with timestamps in that's also a bug in the tests.
Thanks James. I filed another bug on the tests
https://github.com/w3c/web-platform-tests/issues/1267

I'm waiting on a try run, then I'm going to push this to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f08e6f090eaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6122db43cad3
https://hg.mozilla.org/mozilla-central/rev/f08e6f090eaf
https://hg.mozilla.org/mozilla-central/rev/6122db43cad3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Keywords: relnote
Release Note Request (optional, but appreciated)
[Why is this notable]: New web developer facing functionality.
[Suggested wording]: Firefox now supports the Resource Timing API, which allows web pages
   to detemine whether their subresources are loading slowly.
[Links (documentation, blog post, etc)]:  Documentation is still to come.
relnote-firefox: --- → ?
Added to the release notes with "Resource Timing API implemented" as wording. this is consistent with that we have done. We will come back on the release notes once the documentation is live.
relnote-firefox: ? → 35+
Depends on: 1079705
This caused a major (10 MiB) regression in memory usage on areweslimyet.com, falling entirely in the "heap-unclassified" category, which means the additional allocations are not covered by any memory reporters. See bug 1079705.

Unfortunately, this regression is large enough that I consider it grounds for backing out and/or disabling of this feature until the cause is understood. Valentin, what are the options for that?
Flags: needinfo?(valentin.gosu)
The pref is dom.enable_resource_timing. Backing out the revisions in comment 32 should do it.
Given that we now collect timing info for all objects in a webpage, I'd say higher memory usage is expected, however 10MiB is indeed a bit large.
How can we report the memory for the resource timing objects?
Flags: needinfo?(valentin.gosu)
https://wiki.mozilla.org/Memory_Reporting explains how to write a memory reporter.

nsCategoryManager is an example of a simple memory reporter:
http://dxr.mozilla.org/mozilla-central/source/xpcom/components/nsCategoryManager.cpp#495
Depends on: 1083228
Depends on: 1113676
Comment hidden (spam)
That said, _did_ the memory regression here ever get addressed?
Flags: needinfo?(valentin.gosu)
The memory regression was addressed in bug 1064706 and the AWSY regression was tracked in bug 1079705. comment 38 doesn't really go into detail, but I'd encourage the reporter to file another bug complete with steps to reproduce the issue. (browser version, platform, etc)
Flags: needinfo?(valentin.gosu) → needinfo?(eitan.online.777)
Comment 38 was just spam that took comment 35 and inserted a URL.
Flags: needinfo?(eitan.online.777)
> The memory regression was addressed in bug 1064706 and the AWSY regression was tracked in
> bug 1079705.

Great, thanks!
You need to log in before you can comment on or make changes to this bug.