Closed
Bug 1002855
Opened 11 years ago
Closed 10 years ago
Turn on Resource Timing
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 35+ |
People
(Reporter: valentin, Assigned: valentin)
References
(Depends on 2 open bugs)
Details
(Keywords: dev-doc-needed, meta, relnote)
Attachments
(2 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•11 years ago
|
||
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)
Comment 2•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
> 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.
Updated•11 years ago
|
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Updated•10 years ago
|
Keywords: dev-doc-needed
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?
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Comment 9•10 years ago
|
||
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?
Updated•10 years ago
|
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
> 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?
Comment 12•10 years ago
|
||
Also, please file a spec bug about the test suite using the non-standard innerText property, though that doesn't affect the test results.
Comment 13•10 years ago
|
||
Oh, and the test should probably test object/embed to. File spec bugs on that as well?
Comment 14•10 years ago
|
||
Also, you probably want to send an intent to ship mail.
Assignee | ||
Comment 15•10 years ago
|
||
(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
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
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.
Comment 19•10 years ago
|
||
Excellent, thank you!
Assignee | ||
Comment 20•10 years ago
|
||
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
Comment 21•10 years ago
|
||
Keywords: checkin-needed
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
I need to update the tests so they don't expect failure. Thanks.
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8497550 -
Flags: review?(bzbarsky)
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
(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!
Updated•10 years ago
|
Attachment #8497550 -
Flags: review?(bzbarsky) → review+
Comment 27•10 years ago
|
||
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)
Assignee | ||
Comment 28•10 years ago
|
||
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
Comment 29•10 years ago
|
||
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.
Assignee | ||
Comment 30•10 years ago
|
||
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.
Assignee | ||
Comment 31•10 years ago
|
||
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f08e6f090eaf
https://hg.mozilla.org/mozilla-central/rev/6122db43cad3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 33•10 years ago
|
||
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:
--- → ?
Comment 34•10 years ago
|
||
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.
Comment 35•10 years ago
|
||
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)
Assignee | ||
Comment 36•10 years ago
|
||
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)
Comment 37•10 years ago
|
||
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
Comment 39•9 years ago
|
||
That said, _did_ the memory regression here ever get addressed?
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
Comment 38 was just spam that took comment 35 and inserted a URL.
Updated•9 years ago
|
Flags: needinfo?(eitan.online.777)
Comment 42•9 years ago
|
||
> 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.
Description
•