62713 - nsIPresState data structures are needlessly bloated

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Eric Pollmann]

The data structures used to store information in session history are needlessly
bloated.  We wrap integers in nsISupports, use strings to represent "0" and "1",
and should use Atoms where available. (thank to Rod Chris and Steve for
researching this)

This is a real memory bloat issue - Jim Roskind recently analyzed bloat
associated with session history and after just 140 page views in winembed,
nsSHEntry's and their constituents were consuming (unbounded) 500KBytes of
memory - which was second only to the (bounded) 2MB in the image cache.
Reducing the size of nsIPresState would help

500KB / 280 entries (rough numbers based on memory) is almost 2KBytes per entry
in SHistory, which is undoubtedly more than the amount needed!  I'll start this
one out with... me - since I'm the culprit.  :)

See related bug 62651, which deals with capping the number of entries in session
history in various interesting ways.

Comment 1

[Johnny Stenback (:jst)]

I'm assuming that every string is stored as PRUnichar data and from looking at
Jim's data I'm guessing that in some cases we end up putting some very long
strings in session history (the biggest nsSHEntry entry was 13k IIRC). So one
simple change we can make is to store strings as UTF8, this would make things
slightly slower but it would cut the size of all strings in half... Just a thought.

Comment 2

[Kevin McCluskey (gone)]

Setting milestone to mozilla0.8

Comment 3

[Eric Pollmann]

Fix checked in - for the most part.  This fix included the two suggestions I
thought would have the largest impact for the least cost:

1) Don't save state unless it is different from the default state.  This yeilded
upwards of 80% reduction of the number of control values saved for me in my
sample browsing sessions.

2) Save strings as UTF8 instead of UCS2.

More work can be done to not wrap each item in a nsISupports structure, but the
current fix combined with the work that is being done to limit the length of
history should make that almost insignificant.

This bug can not be verified (easily) by running the program.

Comment 4

[Brendan Eich [:brendan]]

No patch, r=, or sr= in the bug?

/be

Comment 5

[Eric Pollmann]

Attachment: patch (reverse)

Comment 6

[Chris Petersen]

Marking verified per last comments.