Closed
Bug 837729
Opened 12 years ago
Closed 12 years ago
when Facebook Messenger is enabled, we create textruns for huge chunks of JS source code
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox18 wontfix, firefox19 wontfix, firefox20 fixed, firefox21 fixed)
RESOLVED
FIXED
Firefox 21
People
(Reporter: jfkthame, Assigned: jfkthame)
References
Details
(Whiteboard: [MemShrink][QA-])
Attachments
(5 files)
1.30 KB,
patch
|
Details | Diff | Splinter Review | |
1.24 KB,
patch
|
Gavin
:
review+
markh
:
feedback+
|
Details | Diff | Splinter Review |
1.44 KB,
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.36 KB,
patch
|
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
When Facebook Messenger is enabled, we seem to be creating gfxTextRuns (as if we're trying to do text measurement or rendering) for large pieces of JavaScript source code. This seems quite pointless; it's burning up CPU cycles and considerable amounts of memory to "shape" text runs that AFAICT will never be needed. We don't display this source code as visible text anywhere, so why do we build gfxTextRuns for it?
The attached investigative patch reports any time we call gfxFontGroup::InitTextRun to initialize a run with over 15000 characters - which is pretty rare in normal web browsing.
With Messenger enabled, I consistently hit this during the initial launch of the browser, before even logging in to Facebook or restoring the previous session's tabs. The text run being created is clearly some JS source (perhaps from Facebook's social-provider code?):
### gfxFontGroup::InitTextRun handling a very long text run (19171 chars)!
----------
[] = [];if (typeof window === "undefined") { self = {};
__DEV__ = 0; self["__DEV__"] = 0;window = {};window.setTimeout = function (cb, time) { return setTimeout(cb, time); };window.setInterval = function (cb, time) { return setInterval(cb, time); };window.clearTimeout = function (id) { return clearTimeout(id); };window.clearInterval = function (id) { return clearInterval(id); };window.location = {};window.location.hash = "";window.location.host = "www.facebook.com";window.location.hostname = "ww (...etc...)
----------
Opening the sidebar and logging in to Facebook triggers several more instances of similar behavior:
### gfxFontGroup::InitTextRun handling a very long text run (57381 chars)!
----------
[] = [];if (typeof window === "undefined") { self = {};
__DEV__ = 0; self["__DEV__"] = 0;window = {};window.setTimeout = function (cb, time) { return setTimeout(cb, time); };window.setInterval = function (cb, time) { return setInterval(cb, time); };window.clearTimeout = function (id) { return clearTimeout(id); };window.clearInterval = function (id) { return clearInterval(id); };window.location = {};window.location.hash = "";window.location.host = "www.facebook.com";window.location.hostname = "ww (...etc...)
----------
### gfxFontGroup::InitTextRun handling a very long text run (153210 chars)!
----------
__d("AsyncRequest",["Event","Arbiter","AsyncResponse","Bootloader","CSS","Env","HTTPErrors","JSCC","Parent","Run","ServerJS","URI","UserAgent","XHR","bind","copyProperties","emptyFunction","evalGlobal","ge","goURI","isEmpty","tx"],function(a,b,c,d,e,f){var g=b('Event'),h=b('Arbiter'),i=b('AsyncResponse'),j=b('Bootloader'),k=b('CSS'),l=b('Env'),m=b('HTTPErrors'),n=b('JSCC'),o=b('Parent'),p=b('Run'),q=b('ServerJS'),r=b('URI'),s=b('UserAgent'),t=b('XHR'),u=b('bind'),v=b('copyProperties'),w=b('empty (...etc...)
----------
### gfxFontGroup::InitTextRun handling a very long text run (366844 chars)!
----------
__d("DataSource",["array-extensions","ArbiterMixin","AsyncRequest","TypeaheadUtil","copyProperties","createArrayFrom","createObjectFrom","emptyFunction"],function(a,b,c,d,e,f){b('array-extensions');var g=b('ArbiterMixin'),h=b('AsyncRequest'),i=b('TypeaheadUtil'),j=b('copyProperties'),k=b('createArrayFrom'),l=b('createObjectFrom'),m=b('emptyFunction');function n(o){this._maxResults=o.maxResults||10;this.token=o.token;this.queryData=o.queryData||{};this.queryEndpoint=o.queryEndpoint||'';this.boots (...etc...)
----------
### gfxFontGroup::InitTextRun handling a very long text run (19828 chars)!
----------
__d("AsyncRequest",["Event","Arbiter","AsyncResponse","Bootloader","CSS","Env","HTTPErrors","JSCC","Parent","Run","ServerJS","URI","UserAgent","XHR","bind","copyProperties","emptyFunction","evalGlobal","ge","goURI","isEmpty","tx"],function(a,b,c,d,e,f){var g=b('Event'),h=b('Arbiter'),i=b('AsyncResponse'),j=b('Bootloader'),k=b('CSS'),l=b('Env'),m=b('HTTPErrors'),n=b('JSCC'),o=b('Parent'),p=b('Run'),q=b('ServerJS'),r=b('URI'),s=b('UserAgent'),t=b('XHR'),u=b('bind'),v=b('copyProperties'),w=b('empty (...etc...)
----------
### gfxFontGroup::InitTextRun handling a very long text run (19087 chars)!
----------
__d("Dialog",["array-extensions","Event","Animation","Arbiter","AsyncRequest","Bootloader","Button","ContextualThing","CSS","DOM","Focus","Form","HTML","Keys","Locale","Parent","Run","Style","URI","UserAgent","Vector","bind","copyProperties","createArrayFrom","emptyFunction","getObjectValues","getOverlayZIndex","tx"],function(a,b,c,d,e,f){b('array-extensions');var g=b('Event'),h=b('Animation'),i=b('Arbiter'),j=b('AsyncRequest'),k=b('Bootloader'),l=b('Button'),m=b('ContextualThing'),n=b('CSS'),o= (...etc...)
----------
One of those runs apparently contains 360K of minified JS source... creating a textRun for this will take around 1.5MB of RAM just for the run itself, in addition to various other overhead.
We need to figure out why we're building these textRuns, and stop doing it; we're wasting millions of CPU cycles and megabytes of space here.
Assignee | ||
Comment 1•12 years ago
|
||
The JS source code appearing in these runs doesn't seem to live anywhere in mozilla-central, AFAICT, so I suspect it's source that comes from Facebook. But why are textruns being created for it? I guess that could either be a problem with the SocialAPI code on our side, or some kind of poor coding on theirs. But either way, it's a bad thing.
Comment 2•12 years ago
|
||
The "FrameWorker" component (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm) uses an iframe in the hidden window to load the JS for the provider's "Worker". We need the iframe because we need some APIs that are associated with windows (XHR, WebSocket, etc.), and we need it to have the provider's origin, and so we also use it to load the JS (and then just take the document's textContent and run it in a sandbox). This is almost certainly where these textruns are coming from.
This iframe is always hidden, though, and so ideally we wouldn't waste memory on anything presentation-related. Is there any way we can avoid this overhead, e.g. by styling the document somehow?
Assignee | ||
Comment 3•12 years ago
|
||
Aha, that's where they come from. Setting display:none on the iframe that's created will save us doing a ton of pointless work, then.
Assignee | ||
Updated•12 years ago
|
Attachment #709807 -
Flags: review?(gavin.sharp)
Comment 4•12 years ago
|
||
Comment on attachment 709807 [details] [diff] [review]
don't create text runs for JS source code loaded in a hidden-window iframe
Looks good to me, though I haven't tested it. Flagging MarkH in case he can think of anything this might negatively affect. I suspect we'll want to uplift this to Aurora at least.
Attachment #709807 -
Flags: review?(gavin.sharp)
Attachment #709807 -
Flags: review+
Attachment #709807 -
Flags: feedback?(mhammond)
Assignee | ||
Comment 5•12 years ago
|
||
(Though I wonder whether inserting the JS source text into the DOM in the first place isn't perhaps an inefficient way to do this?)
Comment 6•12 years ago
|
||
We need to load something from the provider's domain, I think. That could in theory be a lighter-weight document, but with the iframe display:none I suspect the difference would be negligible. We could kill the text in the DOM after we've extracted it, though.
Assignee | ||
Comment 7•12 years ago
|
||
Seems like this should reduce ongoing memory footprint a little further, then.
Assignee | ||
Updated•12 years ago
|
Attachment #709822 -
Flags: review?(gavin.sharp)
Updated•12 years ago
|
Attachment #709822 -
Flags: review?(gavin.sharp) → review?(mhammond)
Comment 8•12 years ago
|
||
drive by...both these patches pass tests for me locally.
Updated•12 years ago
|
Attachment #709807 -
Flags: feedback?(mhammond) → feedback+
Comment 9•12 years ago
|
||
Comment on attachment 709822 [details] [diff] [review]
followup - discard JS source text from the DOM once it has been extracted
awesome!
Attachment #709822 -
Flags: review?(mhammond) → review+
Updated•12 years ago
|
Assignee: nobody → jfkthame
Blocks: 805246
Status: NEW → ASSIGNED
status-firefox18:
--- → wontfix
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 10•12 years ago
|
||
Folded these two (one-line) patches into a single changeset on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdf53493d31
Target Milestone: --- → Firefox 21
Updated•12 years ago
|
Whiteboard: [MemShrink]
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Comment 12•12 years ago
|
||
These patches seem very simple and low risk. Johnathan, can you nominate for aurora? I'm not sure if it is too late to make the last beta for 19.
Assignee | ||
Comment 13•12 years ago
|
||
Combined patch, as landed on central, ready for aurora uplift if approved.
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 710323 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): SocialAPI
User impact if declined: enabling Social features is less performant than it should be: we perform unnecessary layout on huge pieces of JS source code, wasting cycles and churning megabytes of RAM for no purpose
Testing completed (on m-c, etc.): tested locally, now in m-c
Risk to taking this patch (and alternatives if risky): trivial patch with minimal risk
String or UUID changes made by this patch: none
Attachment #710323 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•12 years ago
|
||
As above but for mozilla-beta; the actual code change is identical, but patch didn't apply cleanly because of error-handling changes in the context, so this is the rebased version for beta.
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 710326 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.
[Approval Request Comment]
As per comment 14.
Attachment #710326 -
Flags: approval-mozilla-beta?
Updated•12 years ago
|
Comment 17•12 years ago
|
||
Comment on attachment 710326 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.
Gavin agrees that this isn't critical enough to fix in FF19, given how close we are to release. Approving for Aurora, however.
Attachment #710326 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•12 years ago
|
Attachment #710323 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/5385979f826d
How can QA verify this fix?
Assignee | ||
Comment 20•12 years ago
|
||
I'm not sure you can, really. The goal here is to avoid some unnecessary work during startup and especially when opening the Facebook sidebar. So there'll be a perf improvement, but it will be impossible to detect that reliably among the overall operation of opening the Facebook sidebar.
You -might- be able to detect the reduction in memory footprint (at least a couple of MB) in about:memory, but again, I suspect it'll be hard to spot that among the "noise" of other variations from one run to another.
Comment 21•12 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #20)
In this case can you confirm this is fixed? So we can move it from Resolved Fixed to Verified.
Assignee | ||
Comment 22•12 years ago
|
||
I can confirm that the code is present in mozilla-central and aurora repositories. Also, in a local debug build that includes the original diagnostic patch from comment #0, the huge textruns are no longer reported. So I think that confirms it is working as expected.
Don't think there's much else you can do here - you can't directly "confirm" it with a standard release build because the instrumentation that reveals the huge textruns is not present.
Comment 23•12 years ago
|
||
Based on Comment 20 and Comment 22 marking as [qa-]
Whiteboard: [MemShrink] → [MemShrink][QA-]
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•