49589 - Form Manager adds 490K of bloat to product

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Mike Pinkerton (not reading bugmail)]

I did some quick calculations and I think i've stumbled onto something causing a 
lot of bloat: the wallet form manager tables.

Building these tables allocates about 1700 wallet_MapElement's (at least, given 
the current tables). Each element has 2 (two) nsAutoStrings w/in them. The size 
of an nsAutoString is approx 144 bytes.

1700 * 2 * 144 = 489K

I don't think there is any way to _not_ create these tables, and they live the 
entire length of the application. This looks like a good candidate for 
optimization if we're trying to reduce bloat.

Comment 1

[Mike Pinkerton (not reading bugmail)]

adding keywords, this is a big issue for embedding and footprint.

Comment 2

[Judson Valeski]

Can we get a plus on this? Shouldn't these tables only be created if 
wallet/singlesignon is enabled?

Comment 3

[Stephen P. Morse]

The tables are needed on every form submission so that the wallet code can 
determine if that form is one for which the user would want to capture the data. 

Comment 4

[Stephen P. Morse]

Jud,

Yes, if the wallet code is disabled in the pref panel, these tables are not 
automatically read in.  In that case they will be read in only if the user 
expicitly requests a wallet function.

Comment 5

[Judson Valeski]

Perhaps we can optimize the table entry struct (someone please point out where
this struct lives)? Can we SetCapacity(some better known length) on the
autostrings?

Steve, what if the wallet and signon modules aren't even included in a dist?
Does this code live in those modules, or in the app somewhere?

Comment 6

[Stephen P. Morse]

The code lives in extensions/wallet/src/wallet.cpp.

Comment 7

[Simon Fraser [no longer active]]

Change component

Comment 8

[Warren Harris]

I see nsAutoStrings in several structs in that file. This is bad because it 
causes unnecessary bloat. nsString is the right thing for structs/classes.

I also see nsAutoStrings in argument lists. This is also wrong -- auto strings 
should be an implementation detail, not part of an abstraction. There's nothing 
interesting about an auto string except the constructor, so they should never 
appear in argument lists.

Also, I see static nsAutoStrings which is bad -- they'll never get destroyed on 
shutdown. Best to put these in a global object.

Comment 9

[Judson Valeski]

Also, if we know (steve?) what the lengths of these text entries are, we could 
set the size of the string from the outset and thus reduce mem there too 
(assuming the strings are smaller than the default len of 64 bytes).

Comment 10

[Stephen P. Morse]

Changed the nsAutoStrings in the wallet tables to nsStrings.  Also did a 
setCapacity when putting each nsString into the tables so that the string is 
exactly the size required (no excess).  This should reduce this bloat.

Comment 11

[Mike Pinkerton (not reading bugmail)]

as i said in email, the static autoStrings were to avoid the creation of 6000 
autoStrings, each being set to a higher capacity after creation (forcing 
mallocs). I changed this to a handful (like 6) of static strings to speed up this 
operation on mac by about 1/3 of a second. 

My fix was quick and dirty, but don't blame the messenger. That code needs 
serious attention.