Last Comment Bug 891968 - We have hundreds of copies of the UA string override on B2G
: We have hundreds of copies of the UA string override on B2G
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Dão Gottwald [:dao]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 896382
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-10 10:22 PDT by Andrew McCreight [:mccr8]
Modified: 2013-07-27 01:59 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.98 KB, patch)
2013-07-24 01:20 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (2.89 KB, patch)
2013-07-24 01:38 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (2.85 KB, patch)
2013-07-24 16:11 PDT, Dão Gottwald [:dao]
jduell.mcbugs: review+
hurley: feedback+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2013-07-10 10:22:16 PDT
I'm not sure if this is the right component, and I'm not sure how big of a deal this is, but it looks like the user agent override is holding alive a few hundreds copies of the UA string 'Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1'.  It would be good if we could avoid this somehow.

Here is a list of paths keeping alive one particular copy of this string. The other ones look similar, except with a different URL in the final step.  Let me know if you have trouble understanding this.

via resource://gre/modules/UserAgentOverrides.jsm :
0x4481a5a0 [FakeBackstagePass <no private>]
    --[UserAgentOverrides]-> 0x48e7dd60 [Object <no private>]
    --[getOverrideForURI]-> 0x48ebdd60 [Function uao_getOverrideForURI]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via XPCWrappedNative::mFlatJSObject :
0x48e46040 [ChromeWindow 48c77160]
    --[UserAgentOverrides]-> 0x48e4b5e0 [Proxy <no private>]
    --[private]-> 0x48e7dd60 [Object <no private>]
    --[getOverrideForURI]-> 0x48ebdd60 [Function uao_getOverrideForURI]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via nsXPCWrappedJS[nsIObserver,0x4a040400:0x47de6100].mJSObj :
0x48ebdcc0 [Function buildOverrides]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via nsXPCWrappedJS[nsIObserver,0x4a040480:0x47de6140].mJSObj :
0x48ebdce0 [Function HTTP_on_modify_request]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]
Comment 1 Dão Gottwald [:dao] 2013-07-24 01:20:39 PDT
Created attachment 780266 [details] [diff] [review]
patch

With http://mxr.mozilla.org/gaia/source/build/ua-override-prefs.js?raw=1 applied on desktop, about:memory gave me the following numbers.

before:

73,504 B (00.04%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──49,488 B (00.03%) -- gc-heap
│  ├──20,360 B (00.01%) -- shapes
│  │  ├──11,320 B (00.01%) ── tree/global-parented
│  │  └───9,040 B (00.01%) ── dict
│  ├──16,448 B (00.01%) ── objects/function
│  └──12,680 B (00.01%) ── sundries
├──14,800 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables

after:

63,584 B (00.04%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──43,584 B (00.03%) -- gc-heap
│  ├──16,512 B (00.01%) ── objects/function
│  ├──15,632 B (00.01%) ── sundries
│  └──11,440 B (00.01%) ── shapes/tree/global-parented
├──10,784 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables

Is this the difference you'd expect?
Comment 2 Dão Gottwald [:dao] 2013-07-24 01:38:10 PDT
Created attachment 780269 [details] [diff] [review]
patch v2

cleaned it up a bit. new "after" measurement:

63,320 B (00.03%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──43,416 B (00.02%) -- gc-heap
│  ├──16,512 B (00.01%) ── objects/function
│  ├──15,504 B (00.01%) ── sundries
│  └──11,400 B (00.01%) ── shapes/tree/global-parented
├──10,688 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables
Comment 3 Dão Gottwald [:dao] 2013-07-24 01:43:20 PDT
My patches apply on top of bug 896382's patch, which I just landed on inbound.
Comment 4 Andrew McCreight [:mccr8] 2013-07-24 11:12:58 PDT
From the memory report there, it looks like you are saving 10k in shapes/dict, which I think would be the result of switching from an object to a map.  So that's good, but it doesn't involve the strings per se.  I don't actually see strings in that bit of the report at all.

If you get me a GC heap dump (using this https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump ) then I can see if there are less strings floating around, or you can look at it yourself using this script: https://github.com/amccreight/heapgraph/blob/master/gc/stringy.py
Comment 5 Dão Gottwald [:dao] 2013-07-24 14:53:12 PDT
(In reply to Andrew McCreight [:mccr8] from comment #4)
> If you get me a GC heap dump (using this
> https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump )

I was unable to locate the log file. No idea where it was written to. It's not in the path Firefox was running from.
Comment 6 Andrew McCreight [:mccr8] 2013-07-24 14:58:33 PDT
It should print out the place it dumps it to in the Browser Console.  On OSX, I think it ends up in ~/Library/Caches/TemporaryItems.  On Linux it is in /tmp I think
Comment 7 Dão Gottwald [:dao] 2013-07-24 15:04:40 PDT
(In reply to Andrew McCreight [:mccr8] from comment #6)
> It should print out the place it dumps it to in the Browser Console.

I just got "undefined" back.

> On Linux it is in /tmp I think

Ok, got it.
Comment 8 Dão Gottwald [:dao] 2013-07-24 15:10:44 PDT
Created attachment 780622 [details]
gc-edges.3608.log.bz2
Comment 9 Andrew McCreight [:mccr8] 2013-07-24 15:23:29 PDT
Yeah, I only see 19 strings in there that starting with "Mozilla", so that looks like an improvement, assuming what I was seeing before wasn't an artifact of the leak, which seems totally reasonable.  Thanks.
Comment 10 Dão Gottwald [:dao] 2013-07-24 16:11:39 PDT
Created attachment 780671 [details] [diff] [review]
patch v3

more cleanup
Comment 11 Dão Gottwald [:dao] 2013-07-25 12:29:00 PDT
(In reply to Andrew McCreight [:mccr8] from comment #9)
> Yeah, I only see 19 strings in there that starting with "Mozilla", so that
> looks like an improvement, assuming what I was seeing before wasn't an
> artifact of the leak, which seems totally reasonable.  Thanks.

I looked at the log myself and compared it with a log from an unpatched build. You definitely didn't see a leak and the patch brings UserAgentOverrides.jsm on b2g down to three strings (one for DEFAULT_UA, two in the gOverrides map).
Comment 12 Nicholas Hurley [:nwgh][:hurley] 2013-07-25 13:18:55 PDT
Comment on attachment 780671 [details] [diff] [review]
patch v3

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

This all seems reasonable to me, but given my lack of status as a necko peer, I'm going to hand off to Jason for the official review.

Jason - this touches code from the other user agent override patch I looked at earlier this week, so don't feel like you have to go over this with a fine-toothed comb or anything. I'm just following protocol :)
Comment 13 Jason Duell [:jduell] (needinfo me) 2013-07-25 15:33:06 PDT
Comment on attachment 780671 [details] [diff] [review]
patch v3

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

Seems fine especially since nick thinks so :)
Comment 14 Dão Gottwald [:dao] 2013-07-26 00:41:03 PDT
https://hg.mozilla.org/integration/fx-team/rev/37567e5bbe9f
Comment 15 Dão Gottwald [:dao] 2013-07-26 23:22:56 PDT
Apparently this got merged yesterday.

https://hg.mozilla.org/mozilla-central/rev/37567e5bbe9f

Note You need to log in before you can comment on or make changes to this bug.