We have hundreds of copies of the UA string override on B2G

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: dao)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 attachment, 2 obsolete attachments)

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]
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee: nobody → dao
Posted patch patch (obsolete) — Splinter Review
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?
Attachment #780266 - Flags: feedback?(continuation)
Posted patch patch v2 (obsolete) — Splinter Review
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
Attachment #780266 - Attachment is obsolete: true
Attachment #780266 - Flags: feedback?(continuation)
Attachment #780269 - Flags: feedback?(continuation)
My patches apply on top of bug 896382's patch, which I just landed on inbound.
Depends on: 896382
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
(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.
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
(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.
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.
Posted patch patch v3Splinter Review
more cleanup
Attachment #780269 - Attachment is obsolete: true
Attachment #780269 - Flags: feedback?(continuation)
Attachment #780671 - Flags: review?(hurley)
(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 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 :)
Attachment #780671 - Flags: review?(jduell.mcbugs)
Attachment #780671 - Flags: review?(hurley)
Attachment #780671 - Flags: feedback+
Comment on attachment 780671 [details] [diff] [review]
patch v3

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

Seems fine especially since nick thinks so :)
Attachment #780671 - Flags: review?(jduell.mcbugs) → review+
Apparently this got merged yesterday.

https://hg.mozilla.org/mozilla-central/rev/37567e5bbe9f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.