Closed Bug 811247 Opened 12 years ago Closed 12 years ago

Create only one instance of each social ambient panel instead of one per window

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(firefox18 verified, firefox19 verified)

RESOLVED FIXED
Firefox 20
Tracking Status
firefox18 --- verified
firefox19 --- verified

People

(Reporter: rik, Assigned: Felipe)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(5 files, 3 obsolete files)

Just by enabling "Facebook Messenger for Nightly", I go from 555.70MB to 728.44MB in explicit allocations in about:memory. I have 5 windows opened, two being reduced. I see 15 compartments of more than 8MB.
Summary: Just enabling the SocialAPI takes xx mb per window → Just enabling the SocialAPI takes 35MB per window
Depends on: 805246
Summary: Just enabling the SocialAPI takes 35MB per window → Just enabling the SocialAPI takes xx mb per window
Attached file With SocialAPI
Summary: Just enabling the SocialAPI takes xx mb per window → Just enabling the SocialAPI takes 35MB per window
Attached file Without SocialAPI
Whiteboard: [MemShrink]
Blocks: 805246
No longer depends on: 805246
Is there a separate SocialAPI thing for each tab?
Thanks for the concise bug report, Anthony!

This memory consumption is terrible.
Whiteboard: [MemShrink] → [MemShrink:P1]
Summary: Just enabling the SocialAPI takes 35MB per window → Just enabling the SocialAPI takes 35MB per native (top-level) window
> Is there a separate SocialAPI thing for each tab?

There seems to be one per browser window.
With Facebook Messenger, each browser window will have:
1 sidebar frame
3 iframes (1 for each [friends,messages,notifications] panel)
1 flyout panel with an embedded iframe

The worker is shared between browser windows. AFAIK, we don't have a way to share these iframes between the browser windows. The flyout and notification panel iframes *could* be shared without any user impact if there was a way to do so.
Two thoughts on how to fix the issue for this bug.

1. We could move these panel iframes to be on the hidden window, then use swapDocShell when showing these. This would keep the memory from growing for each browser window.

2. We could stop preloading the notification panels if the notification count is equal to zero. This would be based on the assumption that users will only click on the notification panel if they receive a new notification.
Using the hidden window for panels makes sense - we should do it.

The vast majority of our users don't use multiple windows, so the high memory use caused by this inefficiency shouldn't be very common in practice.
(Not pre-loading until there's a notification may make sense, depending on the loading speed and UX impact that has - we should evaluate that in a separate bug.)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> (Not pre-loading until there's a notification may make sense, depending on
> the loading speed and UX impact that has - we should evaluate that in a
> separate bug.)

I filed bug 811643 to investigate this.
Since this is not a meta-bug (and there's already a meta-bug for tracking social memory usage) I'll rename this one to make it one concrete action that we can do to reduce per-window memory usage, as described in comment 7, and I'll take a stab at it.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Summary: Just enabling the SocialAPI takes 35MB per native (top-level) window → Create only one instance of each social ambient panel instead of one per window
Attached patch Patch v1 (obsolete) — Splinter Review
After quite a while trying to think of the best way to implement this, and then simplyfing to make it into a generic module with an API, I think I got a simple design that works well. It is as follows:

- When a new social panel is created (named "social-messages-jewel" for example), a regular iframe is created in the window and appended to the panel, and the src attribute is set to load its content, exactly the same as it works now.

- Next time a panel of the same kind ("social-messages-jewel") needs to be created (because another window was open), the iframe is created but it doesn't load the content, it just sits there with a blank doc as a placeholder.

- Whenever the toolbar button is clicked to show the panel with the placeholder, that panel takes ownership of the content: we find the frame where the real content is and swap their docshells. With that, only one frame of each kind is loaded at a time.

- If the window with the real content is closed, the next time a panel tries to take ownership of that panel kind, the content gets reloaded
(All refs are held as weakrefs which means it doesn't prevent the content from going away with the window.)


Note that this doesn't use the hidden window at all; due to various platform limitations and differences I decided not to use it. Also the way the module works makes bug 811643 trivial to implement.


I need to write tests, more comments, and look into a test failure, but this is quite close to be ready now. I sent an slightly earlier version of this patch to tryserver if anyone wants to give it a try: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/felipc@gmail.com-2b8a00c0ae8f/
(In reply to :Felipe Gomes from comment #12)
> Note that this doesn't use the hidden window at all; due to various platform
> limitations and differences I decided not to use it.

This sounds good. Should help with dealing with per-window PB mode too, right, since we can just have separate buckets for PB-panels and non-PB-panels?
yeah, we can just assign a different group name to the panels created on PB windows (e. g. "social-messages-jewel-pb") and it should just work
Comment on attachment 687705 [details] [diff] [review]
Patch v1

Review of attachment 687705 [details] [diff] [review]:
-----------------------------------------------------------------

nit: some trailing whitespace in the patch

::: browser/modules/CrossWindowFrame.jsm
@@ +24,5 @@
> +
> +function UNLOADED_URL(aStr) "data:text/html,<!-- Unloaded frame " + aStr + " -->";
> +
> +this.CrossWindowFrame = {
> +  createFrame: function CWF_create(aName, aParent, aFrameAttributes, aPreload = true) {

nit, no longer need to name these functions as the debugger can now pull in the "createFrame" function name starting in Firefox 17.

@@ +32,5 @@
> +      frame.setAttribute(key, val);
> +    }
> +
> +    let src = aFrameAttributes.src;
> +    if (!src) {

When would the frame attributes not contain a "src"? If the related one's window got closed, then this window would never be able to reload it. I don't think this will happen in practice, but we should put a Cu.reportError here to notify developers if the frame attributes don't contain the src.

@@ +61,5 @@
> +  },
> +
> +  borrow: function CWF_borrow(aName, aTargetFrame) {
> +    let miv = Frames.get(aName);
> +    let frame = miv.activeFrame;

This and other places probably need to check for !miv before dereferencing the weak pointer.
Attachment #687705 - Flags: feedback+
> @@ +32,5 @@
> > +      frame.setAttribute(key, val);
> > +    }
> > +
> > +    let src = aFrameAttributes.src;
> > +    if (!src) {
> 
> When would the frame attributes not contain a "src"? If the related one's
> window got closed, then this window would never be able to reload it. I
> don't think this will happen in practice, but we should put a Cu.reportError
> here to notify developers if the frame attributes don't contain the src.

It should be an acceptable use of the API to create the frames ahead of time before knowing their src attribute, which will just create all of them as unloaded. Then when the src is determined, the consumer can call .updateURL() and also .preload() if wanted to then load it in one of the hosts (or not call preload and let it be loaded when borrowed)

> 
> @@ +61,5 @@
> > +  },
> > +
> > +  borrow: function CWF_borrow(aName, aTargetFrame) {
> > +    let miv = Frames.get(aName);
> > +    let frame = miv.activeFrame;
> 
> This and other places probably need to check for !miv before dereferencing
> the weak pointer.

I did this intentionally as I want the API to only allow being called with valid group names. activeFrame is a getter that checks the reference, the only requirement is for aName to have already been created.
Attached patch Module (obsolete) — Splinter Review
This patch is basically the same as before but with a lot more comments and a small bug fix (typo in a property's name) that was causing an existing test to fail
Attachment #687705 - Attachment is obsolete: true
Attachment #689222 - Flags: review?(gavin.sharp)
Attached patch Tests (obsolete) — Splinter Review
And many tests covering the module. Took a bit longer than expected to write those due to a bunch of small details that had to be worked around to make the tests pass. I ran all tests on on mac and a subset on windows, but I still need to give this a full tryserver run. But both patches should be good for review.
Attachment #689223 - Flags: review?(gavin.sharp)
Attachment #689222 - Flags: review?(jaws)
Attachment #689223 - Flags: review?(jaws)
Comment on attachment 689222 [details] [diff] [review]
Module

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>@@ -758,32 +763,42 @@ var SocialToolbar = {

>-        notificationFrames.appendChild(notificationFrame);

This makes "notificationFrames" unused.

>   showAmbientPopup: function SocialToolbar_showAmbientPopup(aToolbarButtonBox) {

>+    let wasAlive = CrossWindowFrame.getIsAlive(notificationFrameId);
>+    CrossWindowFrame.borrow(notificationFrameId, notificationFrame);

>     panel.addEventListener("popupshown", function onpopupshown() {

>-      if (notificationFrame.contentDocument.readyState == "complete") {
>+      if (notificationFrame.contentDocument.readyState == "complete" && wasAlive) {

Not sure I understand why wasAlive is needed, can you explain?

>diff --git a/browser/modules/CrossWindowFrame.jsm b/browser/modules/CrossWindowFrame.jsm

>+this.CrossWindowFrame = {

>+    if (!miv) {
>+      // If this named frame is the first of its group, start tracking
>+      // it. And also load it now if aPreload is true.
>+      miv = new _MultiInstanceView(src, aPreload && frame);

Passing "aPreload && frame" is kind of confusing. I'd prefer (aPreload ? frame : null), or passing both aPreload and aFrame and having the _MultiInstanceView constructor figure it out.

>+      frame.setAttribute("src", aPreload ? src : UNLOADED_URL(aGroup));

Though, rather than having aPreload, could you instead just have the caller not pass in a "src" if it doesn't want preloading? Would simplify things a bit, and we don't even have a use case for not preloading atm, right? Maybe we should remove support for that entirely, and do something like:

if (miv) {
  // wake up previously dead miv if needed
  if (!miv.isAlive)
    miv.revive(src, frame); // sets miv.url, miv.activeFrame, and triggers load (possibly handling empty "src"?)
} else {
  // create new miv, calls revive?
}

This may not work for some reason, but it feels nice in my head, conceptually :) Revive would replace "preload", I think.

>+_MultiInstanceView.prototype = {
>+  get isAlive() {
>+    let frame = this.activeFrame;
>+    return !!(frame &&
>+              frame.contentDocument &&
>+              frame.contentDocument.location);

Hmm, null-checking location is kind of weird. When would it be null? Should this check == this.url?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Comment on attachment 689222 [details] [diff] [review]
> Module
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
> 
> >@@ -758,32 +763,42 @@ var SocialToolbar = {
> 
> >-        notificationFrames.appendChild(notificationFrame);

right, i'll remove it

> 
> This makes "notificationFrames" unused.
> 
> >   showAmbientPopup: function SocialToolbar_showAmbientPopup(aToolbarButtonBox) {
> 
> >+    let wasAlive = CrossWindowFrame.getIsAlive(notificationFrameId);
> >+    CrossWindowFrame.borrow(notificationFrameId, notificationFrame);
> 
> >     panel.addEventListener("popupshown", function onpopupshown() {
> 
> >-      if (notificationFrame.contentDocument.readyState == "complete") {
> >+      if (notificationFrame.contentDocument.readyState == "complete" && wasAlive) {
> 
> Not sure I understand why wasAlive is needed, can you explain?

Yeah, if the frame is not alive, it means that the UNLOADED_URL is loaded and the readyState is complete. However we still need to attach the load listener to get the proper sizing from the actual document that will be loaded.

> 
> >diff --git a/browser/modules/CrossWindowFrame.jsm b/browser/modules/CrossWindowFrame.jsm
> 
> >+this.CrossWindowFrame = {
> 
> >+    if (!miv) {
> >+      // If this named frame is the first of its group, start tracking
> >+      // it. And also load it now if aPreload is true.
> >+      miv = new _MultiInstanceView(src, aPreload && frame);
> 
> Passing "aPreload && frame" is kind of confusing. I'd prefer (aPreload ?
> frame : null), or passing both aPreload and aFrame and having the
> _MultiInstanceView constructor figure it out.

Will change to ? : to make it clear

> 
> >+      frame.setAttribute("src", aPreload ? src : UNLOADED_URL(aGroup));
> 
> Though, rather than having aPreload, could you instead just have the caller
> not pass in a "src" if it doesn't want preloading? Would simplify things a
> bit, and we don't even have a use case for not preloading atm, right? Maybe
> we should remove support for that entirely, and do something like:
> 
> if (miv) {
>   // wake up previously dead miv if needed
>   if (!miv.isAlive)
>     miv.revive(src, frame); // sets miv.url, miv.activeFrame, and triggers
> load (possibly handling empty "src"?)
> } else {
>   // create new miv, calls revive?
> }
> 
> This may not work for some reason, but it feels nice in my head,
> conceptually :) Revive would replace "preload", I think.

One of the nice things of supporting passing a src attribute and aPreload = false is that the url already gets assigned to the group, and then the first time someone tries to display the frame (by borrowing it), the module can handle that case by itself and automatically load it (as if it's reviving the group).

There's currently no use for it now but with this model it means that the core of implementing bug 811643 is simply passing aPreload = false :) (plus handling preloading when the counter changes).  It doesn't mean we should implement bug 811643 but it's a nice use case for the api.

I believe though that I could change the name from "preload" to "revive" in the current api without any harm to its meaning, and maybe it's more clear

> 
> >+_MultiInstanceView.prototype = {
> >+  get isAlive() {
> >+    let frame = this.activeFrame;
> >+    return !!(frame &&
> >+              frame.contentDocument &&
> >+              frame.contentDocument.location);
> 
> Hmm, null-checking location is kind of weird. When would it be null? Should
> this check == this.url?

Oh this is not for checking this.url, this is due to the use of weakrefs where the document might have already been dead but not entirely GC'ed. I'm almost sure I saw a situation while testing where after being removed, frame.contentDocument still existed as an object, but all of it's properties were gone. Then I opened about:compartments to trigger a gc and the weakref.get() returned null afterwards


--

Not to bikeshed too much but after working more on it and discussing this more I think some of the nomenclature became clearer now, so what do you think about me replacing the cryptic _MultiInstanceView with _CWFGroup?

Another thing is that there's nothing window specific in this code, so "Window" in the name is a bit misleading, but I believe the main use case will always be across windows anyway so I don't think there's much harm in keeping it named that way, but I just wanted to bring it up.
SharedFrame, maybe?
Attached patch Module, v2Splinter Review
Reworked the if block on createFrame to make it more clear, changed the name of the module to SharedFrame and the name of the internal object to _SharedFrameGroup. And also s/miv/group/ which should make things more clear
Attachment #689222 - Attachment is obsolete: true
Attachment #689222 - Flags: review?(jaws)
Attachment #689222 - Flags: review?(gavin.sharp)
Attachment #689495 - Flags: review?(gavin.sharp)
Attached patch Tests, v2Splinter Review
Updated tests to use new names
Attachment #689223 - Attachment is obsolete: true
Attachment #689223 - Flags: review?(jaws)
Attachment #689223 - Flags: review?(gavin.sharp)
Attachment #689496 - Flags: review?(gavin.sharp)
Attachment #689495 - Flags: review?(jaws)
Attachment #689496 - Flags: review?(jaws)
Comment on attachment 689496 [details] [diff] [review]
Tests, v2

Review of attachment 689496 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/test/chrome/sharedframe.xul
@@ +103,5 @@
> +      // that preloading works
> +      let frame4 = SharedFrame.createFrame("group2", box, {src: "http://www.example.com/group2", type: "content"});
> +      let frame5 = SharedFrame.createFrame("group2", box, {src: "http://www.example.com/group2", type: "content"});
> +
> +      yield waitForLoad([frame4, frame5]);

nit: missing your //-------------------------- comment to state a new test beginning.

@@ +139,5 @@
> +
> +      //--------------------------
> +      yield waitForLoad([frame8]);
> +
> +      ok(SharedFrame.getIsAlive("group3"), "aPreload = false works");

I don't think this test comment is correct. It should be "aPreload defaults to true".
Attachment #689496 - Flags: review?(jaws) → review+
Attachment #689495 - Flags: review?(jaws) → review?(mhammond)
Comment on attachment 689495 [details] [diff] [review]
Module, v2

Review of attachment 689495 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me - all my comments are nit about names and I'm happy to leave them to your judgement.

::: browser/modules/SharedFrame.jsm
@@ +122,5 @@
> +   * @param aGroupName   the name of the group
> +   * @param aTargetFrame the frame element to which the content should
> +   *                     be moved to.
> +   */
> +  borrow: function (aGroupName, aTargetFrame) {

I'm not that keen on the name 'borrow' - when reading browser-social, it wasn't obvious what it was doing.  I was thinking along the lines of "setOwner", then noticed the comments talk about "Move document ownership", so a name along those lines might be clearer.

@@ +178,5 @@
> +   * Tells if a group currently have an active element.
> +   *
> +   * @param aGroupName  the name of the group
> +   */
> +  getIsAlive: function (aGroupName) {

maybe "isGroupAlive"?

@@ +188,5 @@
> +   * unless the group's name needs to be reused.
> +   *
> +   * @param aGroupName  the name of the group
> +   */
> +  untrack: function (aGroupName) {

maybe "forgetGroup"?
Attachment #689495 - Flags: review?(mhammond) → review+
> @@ +139,5 @@
> > +
> > +      //--------------------------
> > +      yield waitForLoad([frame8]);
> > +
> > +      ok(SharedFrame.getIsAlive("group3"), "aPreload = false works");
> 
> I don't think this test comment is correct. It should be "aPreload defaults
> to true".

oh yeah thanks, copied from the previous test. will fix


I like the suggestions for the better function names. If there are no objections I'll change to those names before landing.

The patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=4eede27b7003 and https://tbpl.mozilla.org/?tree=Try&rev=f787593e7e69
Attachment #689495 - Flags: review?(gavin.sharp)
https://hg.mozilla.org/mozilla-central/rev/7b0b77c101df
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
These are the two patches as landed in m-c merged for simpler approval request. Not requesting beta for now because the plan was to have it on aurora for a while to get some wider testing before landing on beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): social
User impact if declined: Higher memory usage of social per-window and performance penalties
Testing completed (on m-c, etc.): landed on m-c, has tests
Risk to taking this patch (and alternatives if risky): changes the social panel creation/display code
String or UUID changes made by this patch: none
Attachment #690141 - Flags: review+
Attachment #690141 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
Attachment #690141 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 690141 [details] [diff] [review]
Module and tests merged for approval

So since this landed over the weekend on nightly/aurora and we want broader testing of this change we thought it would be a good idea to get this into beta before the go-to-build tomorrow

[Approval Request Comment]
Bug caused by (feature/regressing bug #): social
User impact if declined: Higher per-window memory usage of socialapi and performance penalties
Testing completed (on m-c, etc.): landed on m-c and aurora, has tests
Risk to taking this patch (and alternatives if risky): changes the social panel creation/display code
String or UUID changes made by this patch: none
Attachment #690141 - Flags: approval-mozilla-beta?
Comment on attachment 690141 [details] [diff] [review]
Module and tests merged for approval

Sounds like FF17 is affected - I'm not confident that our feedback channels this close to release will ensure we hear about regressions from such a large patch. Let's let this ride FF18.
Attachment #690141 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 690141 [details] [diff] [review]
Module and tests merged for approval

(In reply to Alex Keybl [:akeybl] from comment #32)
> Sounds like FF17 is affected - I'm not confident that our feedback channels
> this close to release will ensure we hear about regressions from such a
> large patch. Let's let this ride FF18.

Firefox 17 is still "soft-launched" in terms of Social. One of the things that's been holding us back from ramping the feature up completely are reports of performance issues that this bug's fix helps mitigate, hence the desire to get this into 18.

I think the risk is real, but not unreasonable (isolated to social panels display specifically, and much of the patch size is tests and a new isolated JS module). The benefit seems large enough that I'd like to push for reconsideration!
Attachment #690141 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 690141 [details] [diff] [review]
Module and tests merged for approval

Spoke with Gavin in person - we agreed to take this based upon early SocialAPI user feedback. We're going to be extra diligent here with QA testing and user feedback combing. Gavin plans to provide some tips for Anthony's testing.
Attachment #690141 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: qawanted
QA Contact: anthony.s.hughes
> Spoke with Gavin in person - we agreed to take this based upon early
> SocialAPI user feedback.

Excellent.  This is exactly the kind of thing the MemShrink team was worried about, so it's great to see this being handled quickly.
What kind of things are we looking for on the Feedback side? Just primarily around Social API? What kind of things in particular?
Cheng: most simply, any issues using the "notification panels". For Facebook, they're the panels that appear when clicking "Friend Requests", "Messages" or "Notifications" icons. Most relevant would be issues related to their behavior when multiple Firefox windows are open.
QA did not encounter any regressions due to the fix in this bug during 18.0b4 testing. The changes here also seem to have improved memory usage. Windows and Linux saw a marginal improvement of about 15% and 10% respectively. Incredibly, Mac OSX saw an improvement of approximately 50%.

The raw details can be seen here:
https://wiki.mozilla.org/Releases/Firefox_18/Test_Plan#18.0b4_Multi-window_Changes
Marking this verified for Firefox 19 based on in-testsuite coverage and the fact that we've not found regressions this cycle. Please add qawanted if there's testing needed before we release on Tuesday.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: