46702 - parser token recycler unbounded in size

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Warren Harris]

1) Parser token and string usage
  Description: The parser currently heap allocates small CToken instances and 
keeps them on a recycler list "for performance 
  reasons". The recycler list is currently unbounded in size. Text tokens also 
use nsStrings to store copies of their text data - also 
  heap allocated. The overall memory usage shows up high in bloatblame logs. The 
working set memory usage also stays pretty 
  high because of the unbounded token recycler.
  Module owner: [e-mail]
  Task owner: [e-mail], [e-mail], [e-mail]
  Status: Harish is looking at arena allocating the token objects. Scott is 
working on a segmented string implementation which can 
  deal with substrings that share the underlying buffer for the token strings.

Comment 1

[Warren Harris]

P.S. This was from the footprint meeting 7/26/00.

Comment 2

[Chris Waterson]

harishd: I moved nsFixedSizeAllocator.[h|cpp] out of rdf and into xpcom/ds, so
you can use it for this if you like.

Comment 3

[Warren Harris]

Status 8/9/00 footprint meeting: Plan is to have scc get fragmented string stuff 
into the build. Vidur to do the parser work to integrate it. Scc should focus on 
the nsString methods required by the parser first.

Comment 4

[harishd]

Setting priority of P1.

Comment 5

[harishd]

Attachment: Patch version 1.1 [ Arena based token cache. Also, tokens are refcounted ]

Comment 6

[rickg]

I think there is a better answer here. First, though, the recycler wasn't used 
for "performance reasons" -- it was added to reduce memory usage. 

Here's what I'd do:

1. Add an upper bound to the recycler. It's trivial to do, with the bulk of
   the effort going toward measuring performance.
2. I'd have (text) tokens point to the original buffer rather than copy the text    
   data into strings. 
   Note: other tokens are pretty thin -- and usually dont use heap allocation
   for string data. 
3. Use a segmented buffer for the main parser buffer. Add a min. of 
   synchronization so that tokens that point to the segmented buffer never lose
   the content they point to. (This is a very brief period of time, but it
   can be tricky due to scripts and rentrancy).

Comment 7

[Nisheeth Ranjan]

Harish has the fix in hand but it causing an additional 25K of bloat.  He's 
going to work with waterson to figure that out over today.

Comment 8

[leger]

So what is the net gain here? PDT needs numbers to make proper call here.

Comment 9

[Chris Waterson]

Bringing up a the first browser window leads to about 9Mb of data allocated from
the heap left in memory once all is said and done. (This was measured in a debug
build on Linux using the "live bloat" tool that I describe in
news://news.mozilla.org/39AC57EA.1020501%40netscape.com).

With harishd's fix (which I think has already been checked in), we have 8.3Mb of
data that's still allocated in the heap after the first window comes up. That's
a savings of about 700Kb, or almost 10% to the heap size. 

The next step is to reduce the peak memory size.

CTokens currently use nsString's as their member variables. So even though the
CToken objects themselves are allocated and destroyed en masse, they have member
variables the go out to the heap to allocate space for their buffers. This
causes a "large amount" of transient memory use and data copying. (How much is a
"large amount"? I don't know. I can find out for you if you'd like.)

So, vidur and scc plan to replace the nsString objects with "placeholders" that
look and smell like nsStrings, but really just indirect into the raw necko
buffer that's being parsed. In other words, no data copied, no extra heap
allocation, lower peak memory usage.

Hope that helps.

Comment 10

[harishd]

Token cache work is done. Giving bug to Scott for segmented string work. Scott, 
please reassign this bug to Vidur once you're done. Thanx

Comment 11

[Phil Peterson]

PDT agrees P1, but segmented string work is in, right? Can this go to vidur now?

Comment 12

[Scott Collins]

Yes ... Vidur has requested a few small API changes to the |nsSlidingSubstring| 
and |nsSlidingString| classes; but the classes themselves are done and checked-
in, and he can have this bug.  I'll continue to support him in this effort.

Comment 13

[Michael La Guardia]

marking nsbeta3-.  We won't hold pr3 for this. Nominating for rtm

Comment 14

[selmer (gone)]

Why is this bug totally languishing???  It's too late for patches of this size,
did we actually need this???

Comment 15

[Jim Roskind]

We have all the memory performance stuff we could handle for RTM at this point.
Marking rtm minus.
Note that the first wave of this bug fix landed back in late August.

Comment 16

[bsharma]

updated qa contact.

Comment 17

[vidur (gone)]

Fixed earlier this year.

Comment 18

[bsharma]

Verified on:
build: 2001-07-02-04-Trunk
platform: WinNT

Marking it verified as per above developer comments.