Closed Bug 1112733 Opened 7 years ago Closed 7 years ago

Partially restored tab storage can use a lot of memory

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: erahm, Assigned: erahm)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 1 obsolete file)

In following up on bug 1054660 about "unrestored" using too much memory I found that for some pages the |tabData| associated with the the partially restored tab (__SS_data) is using up several MiB of memory.

The problem seems two-fold:
  1) The data is duplicated.
  2) We're loading tabData before the tab is fully restored, ideally this
     would be lazily loaded.

STR:
1) Open browser
2) Load http://www.samsung.com/us/
3) Open an additional blank tab
4) Close browser
5) Open browser
6) Restore session
7) Measure memory usage w/ about:memory before selecting the samsung tab

You'll find that there are 2 copies of a 1MiB string. By grepping through a gc log I was able to find the offending string and using:
   |find_roots.py gc.log -sp <addr_of_str> |
I was able to determine that the data is being held alive by the sessionstore.

Copy 1: 

> via XPCWrappedNative::mFlatJSObject :[ContentFrameMessageManager 7f04c2f08be0] --[gContentRestore]-->
> [Proxy] --[private]--> [Object] --[resetRestore]--> [Function] --[**UNKNOWN SLOT 0**]--> 
> [Object] --[_tabData]--> [Proxy] --[private]--> [Object] --[storage]--> [Object] 
> --[http://www.samsung.com]--> [Object] --[FSR_SAMSUNGCOM_BLOB]--> 
> [string <length 523973 (truncated)> {"start":141869]

Copy 2:

> via nsXPCWrappedJS::mJSObj :[Object] --[_browserTokenMap]--> [WeakMap 7f04d254c060] 
> --[WeakMap entry key]--> [XULElement] --[__SS_data]--> [Proxy] --[private]--> [Object]
> --[storage]--> [Object] --[http://www.samsung.com]--> [Object] --[FSR_SAMSUNGCOM_BLOB]-->
> [string <length 523973 (truncated)> {"start":141869]
Great investigation, Eric!

(In reply to Eric Rahm [:erahm] from comment #0)
> The problem seems two-fold:
>   1) The data is duplicated.

Yes, the first of those two places is the content process (in an e10s world), the DOMStorage data is sent over and restored. I think we could stop holding onto it once that happened by adding:

|delete tabData.storage| to
http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/browser/components/sessionstore/ContentRestore.jsm#l155

The second place is a tab._SS_data property that we hold onto until the tab has been restored. If it's closed before we take our information from there. We could probably move around that data but not get rid of it completely.

>   2) We're loading tabData before the tab is fully restored, ideally this
>      would be lazily loaded.

With SessionStore loading a JSON file from disk lazy loading isn't really doable currently. We read the file once at startup and keep everything we read in memory.
(In reply to Tim Taubert [:ttaubert] (away Dec 19th, back Jan 5th) from comment #1)
> Great investigation, Eric!
> 
> (In reply to Eric Rahm [:erahm] from comment #0)
> > The problem seems two-fold:
> >   1) The data is duplicated.
> 
> Yes, the first of those two places is the content process (in an e10s
> world), the DOMStorage data is sent over and restored. I think we could stop
> holding onto it once that happened by adding:
> 
> |delete tabData.storage| to
> http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/browser/
> components/sessionstore/ContentRestore.jsm#l155

That seems to have done the trick! about:memory diff, note the reduction of copies:
> │  │  ├──-1.44 MB (-21.48%) -- strings
> │  │  │  ├──-1.98 MB (-29.44%) ++ string(length=516298, copies=2, "{"start":1418952269920,"log":[{"x":-1,"e":17,"d": (truncated))
> │  │  │  ├───0.99 MB (14.72%) ++ string(length=516298, copies=1, "{"start":1418952269920,"log":[{"x":-1,"e":17,"d": (truncated))
> │  │  │  ├──-0.35 MB (-5.24%) ++ string(length=184184, copies=2, "{"myespn":"  <div class=//"toolbar//">   <h4><a cl (truncated))
> │  │  │  ├───0.18 MB (02.62%) ++ string(length=184184, copies=1, "{"myespn":"  <div class=//"toolbar//">   <h4><a cl (truncated))

> The second place is a tab._SS_data property that we hold onto until the tab
> has been restored. If it's closed before we take our information from there.
> We could probably move around that data but not get rid of it completely.

Okay, it seems like we can't avoid that (unless we do some form of lazy loading).

> >   2) We're loading tabData before the tab is fully restored, ideally this
> >      would be lazily loaded.
> 
> With SessionStore loading a JSON file from disk lazy loading isn't really
> doable currently. We read the file once at startup and keep everything we
> read in memory.

I see, so to do lazy loading per tab we'd need to change how we save the data so that we could get meta-data separate from the actual storage, etc. Is that something we'd even consider?
Attachment #8538929 - Flags: review?(ttaubert)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8538929 [details] [diff] [review]
Clear tabData storage after calling restore

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

Stealing the review from Tim since it looks like he's out for the holidays.
Attachment #8538929 - Flags: review?(ttaubert) → review+
Attachment #8538929 - Attachment is obsolete: true
Comment on attachment 8539538 [details] [diff] [review]
Clear tabData storage after calling restore

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

Added r=billm, carrying forward r+
Attachment #8539538 - Flags: review+
Keywords: checkin-needed
Hi Eric, can you provide a try link thanks!
Flags: needinfo?(erahm)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #8)
> Hi Eric, can you provide a try link thanks!

Maybe I should have prefixed it w/ "try: "?

(In reply to Eric Rahm [:erahm] from comment #3)
> https://tbpl.mozilla.org/?tree=Try&rev=44dd6d2db695
Flags: needinfo?(erahm)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1731ab46c546
Keywords: checkin-needed
Whiteboard: [MemShrink] → [MemShrink][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1731ab46c546
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][fixed-in-fx-team] → [MemShrink]
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.